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'm a beginner in JavaScript and I am sure that there is a better way to do it (basic one minute timer from 01:00 to 00:00 running from pageload):

JSFiddle

window.onload=function(){
(function(){
     setInterval(function(){
     cro();
     document.getElementById('unminut').innerHTML="0"+jocminut+":"+jocsecunda1+jocsecunda2;
    },1000);
    }());
}
var jocminut = 1;
var jocsecunda1 = 6;
var jocsecunda2 = 10;
function cro(){
    if(jocsecunda1!=0||jocsecunda2!=0){
        jocsecunda2 -=1;
        while(jocminut==1){
            jocminut-=1;
            jocsecunda1 -=1;
        }   
            while(jocsecunda2==-1){
                jocsecunda1-=1;
                jocsecunda2=9;

            }

    }
}
share|improve this question

3 Answers 3

up vote 10 down vote accepted

You want to be careful with setInterval() and setTimeout(), because javascript runs in only a single thread if the event loop is busy it won't fire the code in setInterval() or setTimeout() exactly as expected. This means your timer could take longer than you expected.

Instead of starting with a counter and decrementing on each interval, you can calculate a time difference and then use setInterval() to refresh the frontend. This will be more accurate.

The following code calculates a completion date and then can calculate how many seconds are remaining.

    //sets a completion date 60 seconds into the future
    // Date.getTime() returns the time in milliseconds
    var completion = new Date(new Date().getTime() + 60000),
    function calculateSecondsRemaining() {
        var now = new Date(),
            //Math.max is used to prevent negative numbers from being returned
            differenceInMilliseconds = Math.max(0, completion.getTime() - now.getTime()),
            differenceInSeconds = Math.floor(differenceInMilliseconds / MILLISECONDS_IN_SECOND);
        return differenceInSeconds;
    }

calculateSecondsRemaining() handles the complex logic that was once in cro(), but I feel that is is more readable. When dealing with time and numbers, tracking each digit can be tedious. I prefer to not deal with it at that level.

I prefer to split logic into multiple functions so that the code is more readable. In your code what jumped out at me is three different bits of logic, calculateSecondsRemaining(), updateDisplay(), and formatNumberTo2Places(). By spliting the functions up, the logic within your setInterval() is very small and very easy to understand.

As a final note, once your timer hits zero, the interval no longer needs to fire. The following code cancel the interval using clearInterval() once the timer has reached zero.

  var refreshIntervalId = setInterval(function () {
        var secondsRemaining = calculateSecondsRemaining();
        updateDisplay(secondsRemaining);
        if (secondsRemaining <= 0) {
            //clear timer once time expired
            clearInterval(refreshIntervalId);
        }
    }, 1000);

All Together

The following is wrapped in a self-invoking anonymous function to prevent spilling code into the global scope.

(function (window, document, undefined) {
    var MILLISECONDS_IN_SECOND = 1000,
        SECONDS_IN_MINUTE = 60,
        timerLength = 60, //seconds
        start = new Date(),
        completion = new Date(start.getTime() + (timerLength * MILLISECONDS_IN_SECOND)),
        pentruminut = document.createElement('span');

    // add elements
    document.body.appendChild(pentruminut);

    function formatNumberTo2Places(n) {
        //this assumes 1 or 2 digit positive numbers
        var buffer = (n > 9) ? "" : "0";
        return buffer + n;
    }

    function calculateSecondsRemaining() {
        var now = new Date(),
            //Math.max is used to prevent negative numbers from being returned
            differenceInMilliseconds = Math.max(0, completion.getTime() - now.getTime()),
            differenceInSeconds = Math.floor(differenceInMilliseconds / MILLISECONDS_IN_SECOND);
        return differenceInSeconds;
    }

    function updateDisplay(secondsRemaining) {
        var minutes = Math.floor(secondsRemaining / SECONDS_IN_MINUTE),
            seconds = Math.floor(secondsRemaining % SECONDS_IN_MINUTE);
        pentruminut.innerHTML = formatNumberTo2Places(minutes) + ":" + formatNumberTo2Places(seconds);
    }

    window.onload = function () {
        var refreshIntervalId = setInterval(function () {
            var secondsRemaining = calculateSecondsRemaining();
            updateDisplay(secondsRemaining);
            if (secondsRemaining <= 0) {
                //clear timer once time expired
                clearInterval(refreshIntervalId);
            }
        }, 1 * MILLISECONDS_IN_SECOND);
    };
}(window, document));
share|improve this answer
1  
Good point about setInterval not being accurate. Not clear whether this matters for the OP's use case; could just be something like "wait a minute before you can download" and nobody cares if it's a few milliseconds off. Also, you might want to just store a timestamp in start so you don't have to call getTime repeatedly. And also the OP wanted the timer to start on page load, this one starts immediately. –  Dagg Jul 7 at 22:00
window.onload=function(){

You may want to use addEventListener if there's any chance you'll need to attach other listeners to the load event (and in case another script uses onload).

document.getElementById('unminut')

Since you have created the element dynamically (as shown in your code on JSFiddle), you already have a reference to it. There's no need to give it an ID and then use getElementById to get a reference.

setInterval(function(){

Any time you call setInterval and don't use the return value, that's a bad sign. You almost always want to be able to clear intervals; in this case, you should clear it when the timer reaches 0 so it doesn't run forever.

var jocminut = 1;
var jocsecunda1 = 6;
var jocsecunda2 = 10;

I'm not sure what you're doing here and below (variable names not being in English don't help), but it could be much cleaner.

addEventListener('load', function () {
    var timer = document.createElement('span'),
        seconds = 60,
        interval = setInterval(function () {
            if (!--seconds) {
                clearInterval(interval);
            }
            timer.innerHTML = "0:" + (seconds < 10 ? '0' : '') + seconds;
        }, 1000);

    timer.innerHTML = "01:00";
    document.body.appendChild(timer);
});

http://jsfiddle.net/L7u26/5/


The above example will actually run for about 1 minute and a quarter of a second because setInterval does not have perfect accuracy (some time is spent executing the code). If you need the timer to be very accurate, you can use setTimeout instead of setInterval, and account for the actual time elapsed since the previous tick when scheduling the next tick. In other words if the actual time elapsed since the last tick was 1003 milliseconds, schedule the next tick for 997 milliseconds from now instead of 1000.

addEventListener('load', function () {
    var ONE_SECOND = 1000,
        lastTick = +new Date(),
        timer = document.createElement('span'),
        seconds = 60;

    function tick() {
        var now = +new Date(),
            nextTick = 2 * ONE_SECOND - (now - lastTick);

        lastTick = now;
        if (--seconds) {
            setTimeout(tick, nextTick > ONE_SECOND ? ONE_SECOND : nextTick);
        }
        timer.innerHTML = "0:" + (seconds < 10 ? '0' : '') + seconds;
    }

    timer.innerHTML = "01:00";
    document.body.appendChild(timer);
    setTimeout(tick, ONE_SECOND);
});

http://jsfiddle.net/L7u26/6/

share|improve this answer
1  
Thanks! This is really useful information. I will name my variable in English from now on. –  Claudiu Jul 7 at 22:09

There's another Stack Overflow question with a useful function:

String.prototype.toHHMMSS = function () {
    var sec_num = parseInt(this, 10); // don't forget the second param
    var hours   = Math.floor(sec_num / 3600);
    var minutes = Math.floor((sec_num - (hours * 3600)) / 60);
    var seconds = sec_num - (hours * 3600) - (minutes * 60);

    if (hours   < 10) {hours   = "0"+hours;}
    if (minutes < 10) {minutes = "0"+minutes;}
    if (seconds < 10) {seconds = "0"+seconds;}
    var time    = hours+':'+minutes+':'+seconds;
    return time;
}

Why are you counting the seconds in units of both 10s and 1s? Why not something like (not tested):

var tick = function(seconds){
               if(seconds == 0) {
                   clearInterval(refresh);
                   return '0';
               } 
               document.getElementById('unminut').innerHTML = seconds.toHHMMSS();
               seconds--;
           };
var seconds = 60;
window.onload=function(){(function(){var refresh = setInterval(function(){tick(seconds);}, 1000)}());}
share|improve this answer
    
This solution never clears the interval after the timer reaches 0. It keeps running the function every second forever. –  Dagg Jul 7 at 21:29
    
Thanks. Edited to add a solution. –  fbt Jul 7 at 21:46
    
Thanks for the answer, but I will move the accept to pgreen2 because it is more documented and explained better the process. –  Claudiu Jul 7 at 22:19
    
@user37605 you should decide how important accuracy is before introducing all the complexity from that answer into your code. A solution like this one or your original one is only going to be off by about a quarter of a second by the time the counter hits 0. If accuracy is not that important, I'd definitely favor a solution like this. In particular if the goal is to get the user to look at an advertisement or something for a minute before they do something else, the answer you selected will be selling you short, since it starts before the page (and advertisement) loads. –  Dagg Jul 7 at 22: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.