4
\$\begingroup\$

I have this timer working already(jsbin):

HTML:

  <input id="m" type="number" value="0" min="0" max="59"/>
  <input id="s" type="number" value="2" min="0" max="59"/>
  <input id="start" type="button" value="Start!" />
  <output>00:00</output>

JavaScript:

var o = document.querySelector('output'),
m = document.querySelector('#m')
s = document.querySelector('#s'),
start = document.querySelector('#start');


start.addEventListener('click', function(){
    var mm = parseInt(m.value),
        ss = parseInt(s.value);
    timer = function(){
        if(mm>0){
            if(ss>0){
                ss--;
            }
            else{
                mm--;
                ss = 59;
            }
            setTimeout("timer()", 1000);
            o.value = (mm<10 ? '0' + mm : mm) + ":" + (ss<10 ? '0' + ss : ss);
        }
        else{
            if(ss>0){
                ss--;
                o.value = "00:" + (ss<10 ? '0' + ss : ss);
                setTimeout("timer()", 1000);
            }
        }
    }

    timer();
}, false);

It seems a lot of code for just a timer. Is it possible to implement this with less code? Maybe with loops, Date object or something else?

This code have problem and that is number of items in call stack when the time is big enough. I tried to write same code with while or for but I couldn't get a good result.

MY QUESTION is how to write this countdown with for or while?

\$\endgroup\$

3 Answers 3

4
\$\begingroup\$

document.querySelector("#id") is just a costly version of document.getElementById("id"). document.getElementById is the fastest way to select an element in your page (to the smartasses of you: besides document.head, document.body etc)

document.querySelector("tagname") is also just a costly version of document.getElementsByTagName("tagname")[0]. Coincidentally, that's the second fastest way to get an element.

document.querySelector and its array-like version are only useful for compound statements that are either too long or too difficult to do with "regular" DOM methods. One might argue that if your html is compound and complex, you're doing it wrong, and they'd most likely be right.

If I were you, I'd also do future-you a favor and name your variables in a way that makes sense.

So, what do we have so far?

var output = document.getElementsByTagName("output")[0],
    minutes = document.getElementById("m"),
    seconds = document.getElementsById("m"),
    start = document.getElementById("start");

Moving on. The event listener is fine, now about parsing integers...

Try this piece of code: parseInt("08"). The result is 0. Why, you ask? Well, if you don't pass a radix as a second parameter, parseInt will be nice enough to guess what you mean. parseInt sees that your string begins in a 0 and, of course, assumes the number is in octal! Why wouldn't it be?

Point: either pass a radix like this: parseInt(string, 10) or use parseFloat, which will always assume a decimal number. Whichever you like best, though I once heard that parseFloat is faster, dunno if that's true. I personally prefer parseFloat, for no obvious reason

So we get:

var mm = parseFloat(minutes.value),
    ss = parseFloat(seconds.value);

Magnificent.

This line:

timer = 

Contains one of the most widely made errors in javascript. It's called implied global.

Basically, when you omit the var statement, the variable definition list gets appended to the global object (which window refers to). So:

function foo() { answer = 42; }
foo();
window.answer === 42;

Never omit the var statement. Even if you do wish to append a variable to the global object, do it explicitly (window.answer = 42;). Better that then let someone scratch their head trying to figure out how magic variables appeared from seemingly nowhere.

Besides one more thing, your code is fine. What's that other thing, you ask? setTimeout

When you pass a string to setTimeout, it's the same as calling eval. It evaluates the string. That's slow and bad. Instead, pass a function to it:

setTimeout(timer, 1000);

Since timer is a function, you can pass it around like any other value.

Now, one might ask: "How do I call func with parameters?"

setTimeout(function() {
    func(something, somethingElse);
}, 1000);
//or
setTimeout(func, 1000, something, somethingElse);

About your actual code? it's fine. It's not the best piece of code I've ever, but it's not the worse as well. It's not too long for a timer, since all that's done is decrementing and calling itself again. I'd move the < 10 checks to their own function, but maybe that's because I'm insane and not that "your way" is worse.


tl;dr Your code contains errors, but other than that, it's fine.

\$\endgroup\$
7
  • \$\begingroup\$ I don't agree with things you said about querySelector because fastness is not a matter here and I just used that selector for ease of code. My main question was how to improve timer that is not answered here. \$\endgroup\$
    – Mohsen
    Oct 22, 2011 at 20:28
  • 1
    \$\begingroup\$ @Mohsen Yes I did. Right at the end. \$\endgroup\$
    – Zirak
    Oct 22, 2011 at 20:30
  • \$\begingroup\$ How much I'm close to your ideal? jsbin.com/udozuk/8/edit \$\endgroup\$
    – Mohsen
    Oct 22, 2011 at 20:49
  • 1
    \$\begingroup\$ @Mohsen ids represent elements which must be unique. If you have several elements which fit under a single, general name, that's a class for you, and there's document.getElementsByClassName. \$\endgroup\$
    – Zirak
    Oct 22, 2011 at 21:00
  • 1
    \$\begingroup\$ getElementsByClassName browser support is same as querySelector while you can do queryselector("#div .class element[attr]") but it's like ten line of code if you do this with other selectors \$\endgroup\$
    – Mohsen
    Oct 23, 2011 at 1:36
4
\$\begingroup\$

You can't use a loop to do something like this, as that would lock up the user interface. There is nothing happening in the browser as long as your function is running, so the change in the output would simply not show up.

Here are some suggestions:

You have different code for counting down when there is minutes involved and when there isn't, but you can use the same code for both.

You can keep track of the time as seconds, and split it into minutes and seconds when you display it, that simplifies the code a bit.

You can use the function name timer in the setTimeout call, that way you don't have to declare the function globally.

document.querySelector('#start').addEventListener('click', function(){

  var o = document.querySelector('output');
  var t =
    parseInt(document.querySelector('#m').value, 10) * 60 +
    parseInt(document.querySelector('#s').value, 10);

  var timer = function(){
    if (t > 0) {
      t--;
      var m = Math.floor(t \ 60);
      var s = t % 60;
      o.value = (m < 10 ? '0' : '') + m + ":" + (s < 10 ? '0' : '') + s;
      window.setTimeout(timer, 1000);
    }
  }

  timer();
}, false);

Regarding the call stack: The code is not recursive, so it won't build up calls on the call stack. The timer function adds itself as an event, it doesn't call itself.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ @Mohsen: Yes, they can be combined, but that's just a matter of preference. It has no impact on performance. \$\endgroup\$
    – Guffa
    Oct 22, 2011 at 21:20
  • \$\begingroup\$ @Guffa : Hie there! I was wondering if there is optimized code for countdown showing DD:HH:MM:SS . Can you please help me out for this? Thanks !! \$\endgroup\$
    – Dharmraj
    Sep 18, 2014 at 13:21
  • \$\begingroup\$ @Dharmraj: For such a long time period you would need to calculate the time as a time difference to get it reasonably accurate, not just using a counter and a timer. Anyway, here are some tips: stackoverflow.com/questions/9642777/… \$\endgroup\$
    – Guffa
    Sep 18, 2014 at 17:32
1
\$\begingroup\$

If you use seconds instead of a mm:ss to hold your time, everything will be simpler.

Plus, you can encapsulate your reading and writing of values:

function readSeconds() {
  return parseInt(m.value, 10) * 60 + parseInt(s.value, 10);
}

function output(seconds) {
  var mm = seconds / 60, ss = seconds % 60;
  o.value = (mm<10 ? '0' + mm : mm) + ":" + (ss<10 ? '0' + ss : ss);
}

Then, you can write your start method more easily. Something like:

start.addEventListener('click', function(){
    var seconds = readSeconds();
    function timer(){
       seconds--;
       output(seconds);
       setTimeout(timer, 1000);
    }
    timer();
}, false);

Obviously this isn't all the details, but a little encapsulation helps quite a bit.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.