Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've written this little countdown script to count until the end of our school days.

Now, that was written quickly, badly remembering modulo from a few years ago, and I don't think that's very optimized.

I thought about updating only the seconds and check if it needs to update the minute too, etc. But it wouldn't be accurate as setTimeout is not (depending if the browser lags, etc).

Here is the code:

(function() {
    var el = document.getElementById('countdown'),
        endDate = new Date('March 30, 2012 18:10:00'),
        curDate,
        diff,
        days,
        hours,
        minutes,
        seconds,
        tmp,
        countdown,
        pad = function(number) { return (number < 10 ? '0' : '') + number }
    ;(function() {
        curDate = new Date()
        diff = new Date(curDate.getTime() - endDate.getTime())
        days = Math.abs(diff / (24 * 60 * 60 * 1000))
        tmp = diff % (24 * 60 * 60 * 1000)
        hours = Math.abs(tmp / (60 * 60 * 1000))
        tmp = tmp % (60 * 60 * 1000)
        minutes = Math.abs(tmp / (60 * 1000))
        tmp = tmp % (60 * 1000)
        seconds = Math.abs(tmp / 1000)
        countdown = pad(parseInt(days, 10)) + ':' + pad(parseInt(hours, 10)) + ':' + pad(parseInt(minutes, 10)) + ':' + pad(parseInt(seconds, 10))
        if ('textContent' in el) el.textContent = countdown
        else el.innerText = countdown
        setTimeout(arguments.callee, 1000)
    }())
}())

PS: about the semicolons missing, it is a deliberate choice as I find this more readable (the ; right before the (function() is because it is the only edge case where the semicolon insertion doesn't work correctly).

Edit : @Zirak has reviewed my code. Here is the reviewed version, which does look way cleaner!

(function() {
    //the variable block here seems kinda weird, but whatever toots your horn
    var el = document.getElementById('countdown'),
        endDate = new Date('March 30, 2012 18:10:00'),
        curDate,
        diff,
        days,
        hours,
        minutes,
        seconds,
        tmp,
        countdown,
        //added Math.floor. this is already a shortcut, might as well make it a double one
        pad = function(number) { return (number < 10 ? '0' : '') + Math.floor(number) },

        //calculate these constants once, instead of over and over
        minute = 60 * 1000,
        hour = minute * 60,
        day = hour * 24

    ;(function tick() {
        curDate = new Date()
        //you want the absolute value of this, not of individual calculations using this
        diff = Math.abs(new Date(curDate.getTime() - endDate.getTime()))
        days = diff / day

        tmp = diff % day
        hours = tmp / hour

        tmp = tmp % hour
        minutes = tmp / minute

        tmp = tmp % minute
        seconds = tmp / 1000

        //parseInt was redundant
        countdown = pad(days) + ':' + pad(hours) + ':' + pad(minutes) + ':' + pad(seconds)

        if ( 'textContent' in el ) {
            el.textContent = countdown
        }
        else {
            el.innerText = countdown
        }

        //dont't use arguments.calle, it's deprecated
        //simply use a named function expression
        setTimeout(tick, 1000)
    }())
}())​
share|improve this question
 
Please make sure you include your code in your question. If you update the code in the future, the existing answers will make no sense. Or maybe you move or delete the repo, or... –  ANeves Mar 23 '12 at 16:48
 
Alright, updated my question :) –  Florian Margaine Mar 23 '12 at 17:02
 
Thanks. You could still leave the link to the repo if you wanted, no harm in it at all; but it is important to include the code itself. –  ANeves Mar 23 '12 at 17:06
 
Some cool stuff going on here, never thoughts of using arguments.callee like that to avoid a named function. though mdn does seem to recommend against it: developer.mozilla.org/en/JavaScript/Reference/… –  George Mauer Mar 23 '12 at 17:19
1  
you wanan get really slick? el[('textContent' in el) ? 'textContent' : 'innerText'] = countdown Also though, you can cache the result of that calculation –  George Mauer Mar 23 '12 at 19:24
show 2 more comments

2 Answers

up vote 1 down vote accepted

Your whole code can be reduced to this.

/* secsToDHMS()
 * Author Gary Green
 * http://stackoverflow.com/users/63523/gary-green
 */
function secsToDHMS(s)
{
  var x = [86400, 3600, 60, 1],z,i=-1;
  while (z=x[++i])
        x[i] = ("0" + parseInt(s / z,10)).slice(-2),
        s %= z;

    return x.join(':');
};

(function() {

    var currentDate      = new Date();
    var endDate          = new Date('March 30, 2012 18:10:00');
    var secsToGo         = (new Date(endDate - currentDate)).getTime() / 1000;
    var countdownElement = document.getElementById('countdown');

    var timer = setInterval(function() {

        if (--secsToGo < 0) {
            clearInterval(timer);
            return;
        }

        countdownElement.innerHTML = secsToDHMS(secsToGo);

    }, 1000);

})();

Fiddle: http://jsfiddle.net/xg63V/4


Compressed version 311 bytes

(function(){var d=(new Date(new Date("March 30, 2012 18:10:00")-new Date)).getTime()/1E3,
f=document.getElementById("countdown"),g=setInterval(function(){if(0>--d)clearInterval(g);
else{for(var a=d,b=[86400,3600,60,1],c,e=-1;c=b[++e];)b[e]=("0"+parseInt(a/c,10)).slice(-2),
a%=c;f.innerHTML=b.join(":")}},1E3)})();​
share|improve this answer
 
Your version looks somehow nice, but I don't like the use of setInterval or innerHTML. The secsToDHMS function is really nice though! –  Florian Margaine Mar 24 '12 at 8:25
add comment

First question - your countdown is "symmetric", it does not care if current date is past the end date. Is it on purpose?

Define constants to avoid "magic numbers": var ONE_DAY = 1000 * 60 * 60 * 24 etc

May be it is better to calculate diff = Math.abs(curDate.getTime() - endDate.getTime()) without new object construction, and you do not need to invoke Math.abs anymore (see the question above).

Then, days = Math.floor(diff / ONE_DAY) to ensure integer number. Same for other calculations.

share|improve this answer
 
Thanks for your advices, they're already used in @Zirak's version :). The symmetric part of the countdown is okay, a simple if check will solve this. –  Florian Margaine Mar 24 '12 at 8:27
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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