current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite

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).

(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)
    }())
}())​

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).

share|improve this question
    
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
    
Yep, it is deprecated :-). I just posted a reviewed version by @Zirak. Not sure if I should close the question or not. –  Florian Margaine Mar 23 '12 at 17:24
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
    
Good point, I should definitely cache the result of this. –  Florian Margaine Mar 24 '12 at 8:27
    
setInterval() is far more optimized and is often recommended over setTimeout(). There is an awesome article by John Resig (inventor of jQuery and co-author of Secrets of the JavaScript Ninja). The book is amazing, I recommend it to everyone too! John Resig's blog –  stevematdavies Dec 11 '14 at 8:06

2 Answers 2

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
up vote 0 down vote

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

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.