Dismiss Notice
Join Physics Forums Today!
The friendliest, high quality science and math community on the planet! Everyone who loves science is here!

Any idea why my JS function isn't working?

  1. Feb 28, 2014 #1
    I made this to convert a time in hh:mm:ss form to seconds. Not working, for some reason. I tested "00:02:29" and got "2". Any idea why? Also, any ideas on how I can make this procedure more simple and elegant?

    Code (Text):

    function clock2secs(clockTime)
    {
           
        if (clockTime.length > 8)
        {
            return -1;
        }
               
        if (isDecimal(clockTime.charAt(0)) && isDecimal(clockTime.charAt(1)))
        {
            var hrs = parseInt(clockTime.substring(0,1));
        } else
        {
            return -1;
        }
               
        if (clockTime.charAt(2) != ':')
        {
            return -1;
        }
               
        if (isDecimal(clockTime.charAt(3)) && isDecimal(clockTime.charAt(4)))
        {
            var mins = parseInt(clockTime.substring(3,4));
        } else
        {
            return -1;
        }
               
        if (clockTime.charAt(5) != ':')
        {
            return -1;
        }
               
        if (isDecimal(clockTime.charAt(6)) && isDecimal(clockTime.charAt(7)))
        {
            var secs = parseInt(clockTime.substring(6,7));
        } else
        {
            return -1;
        }
               
        /* If here, clockTime was in valid format hh:mm:ss, and its
           minutes and seconds were parsed correctly.
        */
        return 3600 * hrs + 60 * mins + secs;
    }

    function isDecimal(c)
    {
        return c <= '9' && c >= '0';
    }
     
     
    Last edited by a moderator: Feb 28, 2014
  2. jcsd
  3. Feb 28, 2014 #2
    I would use the radix parameter in the parseInt calls. I am not sure how parseInt would interpret "08" or "09" without it.
    Otherwise, it looks fine. ... But I'll continue looking for the bug.
    I would have done all of the checking before any of the parseInt's. That way you could put all the if statements under a comment that says "Check proper syntax" and the remaining statements under a separate heading.

    Oh. I would definitely change the first check to "is not equal to 8".
     
  4. Feb 28, 2014 #3

    Mark44

    Staff: Mentor

    I took a ton of tabs out of your code since some lines were wider than the display window.
    There are a couple of things you can do to debug.
    1. Add in some alert() calls, with the value of the variable you're interested in. This causes a window to pop open to display that value.
    2. Use the F12 debugging tools that are available with most modern browsers. These debuggers allow you to set a breakpoint and single-step through your code. They will also show you the local variables and their values.
     
  5. Feb 28, 2014 #4

    Mark44

    Staff: Mentor

    To add to what .Scott said about putting all the sanity check code in one place (like possibly in its own function), you could combine these two if blocks:
    Code (Text):
    if (clockTime.charAt(2) != ':')
    {
        return -1;
    }
     
    Code (Text):
    if (clockTime.charAt(5) != ':'  )
    {
        return -1;
    }

     
    into one if block, like so:
    Code (Text):
    if (clockTime.charAt(2) != ':' || clockTime.charAt(5) != ':')
    {
        return -1;
    }

     
     
  6. Feb 28, 2014 #5
    The problem is with you "substring" parameters. You need to add one to all of you terminating indeces.
     
  7. Feb 28, 2014 #6
    Also, compare your results for "01:01:01" with "09:09:09". Just to demonstrate the problem when you don't specify decimal to parseInt.
     
  8. Feb 28, 2014 #7
    I always thought that it was proper coding to check for errors while in the process of trying to accomplish a task.
     
  9. Feb 28, 2014 #8

    Mark44

    Staff: Mentor

    There's nothing that says you can't check for the errors all at once, and if you don't find any, carry on with the task at hand.
     
  10. Feb 28, 2014 #9
    It should work as is. Look at the specifications of parseInt() http://www.w3schools.com/jsref/jsref_parseint.asp
     
  11. Feb 28, 2014 #10
    Either way is proper coding. The issue is one of presentation - and to a minor extent modularity.
    When you code, you can think of what words you would use to explain what your code is doing. My thought is that it is more awkward and overly detailed to talk about each token in the string - and more fluent to describe it as a two step process, check & convert.

    But either way is completely "proper".

    The other aspect is how you demonstrate your defenses. When you write a function, you often treat the input parameters as coming straight from the devil whose sole purpose is to torture the world through your function. Your defense is either to check all parameters upon entry - or to check each parameter just before using it. In the first case, it is easier to check whether your defense is adequate because all of the checks are done in one place.
     
  12. Feb 28, 2014 #11
    So, this looks good to you?

    Code (Text):

    function clock2secs(clockTime)
    {
        if (isProperTimeForm(clockTime))
        {
            var hrs = parseInt(clockTime.substring(0,2));
            var mins = parseInt(clockTime.substring(3,5));
            var secs = parseInt(clockTime.substring(6,8));
            return 3600 * hrs + 60 * mins + secs;
        } else
        {
            return -1;
        }
    }

     function isProperTimeForm(clockTime)
    {
           return          (clockTime.length == 8)
                && (isDecimal(clockTime.charAt(0)))
                && (isDecimal(clockTime.charAt(1)))
                && (clockTime.charAt(2) == ':')
                && (isDecimal(clockTime.charAt(3))
                && (isDecimal(clockTime.charAt(4))
                && (clockTime.charAt(5) == ':')
                && (isDecimal(clockTime.charAt(6))
                && (isDecimal(clockTime.charAt(7));
    }

    function isDecimal(c)
    {
        return c <= '9' && c >= '0';
    }
     
     
    Last edited: Feb 28, 2014
  13. Feb 28, 2014 #12
  14. Feb 28, 2014 #13
    I like it - or I will after you make that "09:09:09" check.

    --------------------------

    Actually, I just looked it up. The "09:09:09" check may or may not reveal the problem. Here is the issue:
     
  15. Feb 28, 2014 #14
    parseInt("09:09:09") == "9",

    if that's what you were getting at.

    However, I'm only using parseInt() on each the 3 substrings separated by colons.
     
  16. Feb 28, 2014 #15
    Obviously "9" is not the answer. But it's also not what I would have expected.
    I would have expected that with the latest browsers you would have gotten the correct answer (32949) and with other browsers you would have gotten zero.

    Did you put the ",10" in the last parseInt but not the first two?
     
  17. Feb 28, 2014 #16
    It turns out I can't do multiple-line statements like this:

    Code (Text):

        return             (clockTime.length == 8)
                && (isDecimal(clockTime.charAt(0)))
                && (isDecimal(clockTime.charAt(1)))
                && (clockTime.charAt(2) == ':')
                && (isDecimal(clockTime.charAt(3))
                && (isDecimal(clockTime.charAt(4))
                && (clockTime.charAt(5) == ':')
                && (isDecimal(clockTime.charAt(6))
                && (isDecimal(clockTime.charAt(7));
     
    Dang it.
     
  18. Feb 28, 2014 #17
    You're missing some end parenthesis.
     
  19. Feb 28, 2014 #18
    Don't dang it. Put the end parens in. You are missing four of them.

    -------------------------

    And let us know how you're doing.
     
    Last edited: Feb 28, 2014
  20. Feb 28, 2014 #19
    Wow. People as stupid as me should not be allowed to live.

    I got it to work, finally.

    This is one of my first times actually using JavaScript. I'll show you the full idea for my program if I run into any more trouble.

    Test of the clocks2sec function:

    Code (Text):

    document.write(clock2secs("00:02:29"))

    function clock2secs(clockTime)
    {
        if (isProperTimeForm(clockTime))
        {
            var hrs = parseInt(clockTime.substring(0,2));
            var mins = parseInt(clockTime.substring(3,5));
            var secs = parseInt(clockTime.substring(6,8));
            return 3600 * hrs + 60 * mins + secs;
        }
        else
        {
            return -1;
        }
    }

     function isProperTimeForm(clockTime)
    {
        return     (clockTime.length == 8)
                && (isDecimal(clockTime.charAt(0)))
                && (isDecimal(clockTime.charAt(1)))
                && (clockTime.charAt(2) == ':')
                && (isDecimal(clockTime.charAt(3)))
                && (isDecimal(clockTime.charAt(4)))
                && (clockTime.charAt(5) == ':')
                && (isDecimal(clockTime.charAt(6)))
                && (isDecimal(clockTime.charAt(7)));
    }


    function isDecimal(c)
    {
        return c <= '9' && c >= '0';
    }

     
    produces

    Code (Text):

    149
     
     
  21. Feb 28, 2014 #20
    If you want to feel smart, stay away from programming.
    Become a Supreme Court Justice. They're never wrong.

    -------

    You are going to redo the "09:09:09" test, I hope.
     
Know someone interested in this topic? Share this thread via Reddit, Google+, Twitter, or Facebook