0

I have got a piece of code, that should countdown some number (in this example 1111) and write it into span id="timeframe". But it doesn't work, i dont know why. Can you tell me where is my mistake, please? 

<script language="JavaScript">
  <!--
  var txt_minute = "Min";
  var txt_second = "s";
  var time_left = 1111;
    var temp=timeleft;
              houres = Math.floor(temp / 3600); 
              temp %= 3600;
              minutes = Math.floor(temp / 60)";
              temp %= 60;
              seconds= temp;

function display()
{
    time_left -= 1;
    if (seconds <= 0) {
        seconds = 59;
        minutes -= 1;
    } else {
        seconds -= 1;
    }
if (minutes <= 0){
minutes=59;
houres-=1;
}

    if (time_left <= 0) {
        seconds = 0;
    }



    if (time_left > 0) {
        document.getElementById("timeframe").innerHTML = houres +" hours" + minutes + " " + txt_minute + " " + seconds + " " + txt_second;
    } else {
        window.location.reload();
    }
    setTimeout("display()",1000);
}

display();

//-->
</script>
Improve this question
2
  • 2
    What doesn't work? Does it display incorrectly? Completely break? Improper output? Just never show up? Commented Feb 21, 2010 at 16:48
  • If you want your timer to be more accurate, use the Date object and a lower interval frequency. JavaScript timers aren't great for countdowns :-) Commented Feb 21, 2010 at 17:07

5 Answers 5

Reset to default
4

There are some smaller mistakes:

  1. minutes = Math.floor(temp / 60)";
    There is a trailing quotation mark that shouldn't be there.
  2. var temp=timeleft;
    That should be time_left.
  3. Also you should use setTimeout with a reference to the function instead of requiring it to parse the text as code: setTimeout(display,1000);

edit: I wrote a small, clearer countdown script.

function startCountdown ( timeLeft )
{
    var interval = setInterval( countdown, 1000 );
    update();

    function countdown ()
    {
        if ( --timeLeft > 0 )
            update();
        else
        {
            clearInterval( interval );
            update();
            completed();
        }
    }

    function update ()
    {
        hours   = Math.floor( timeLeft / 3600 );
        minutes = Math.floor( ( timeLeft % 3600 ) / 60 );
        seconds = timeLeft % 60;

        alert( '' + hours + ':' + minutes + ':' + seconds );
    }

    function completed ()
    {
        alert( 'completed' );
    }
}
startCountdown( 1234 );

Just alter the update and completed function to your needs and start the countdown with your desired value.

And btw. if you want to change some innerHTML of an element in your code, you should save the reference somewhere, instead of getting it via DOM every call.

Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

the third is cosmetic ;D as a side note to OP : read error messages !! they are most informative (if they aren't someone (probably you) really messed something up)
The third is not cosmetic. You should avoid all forms of eval
@n00b32: It's not entirely cosmetic, the JavaScript interpreter basically has to do an eval on that every time. It's not a huge deal, but it's not just cosmetic.
@T.J. Crowder: i know ;) but in this case it doesn't matter . though you are correct its better to write good code even in simple scripts.
3 
      minutes = Math.floor(temp / 60)";

?

Improve this answer

Comments

1

All the typos that have been mentioned above and one logic flaw ..

If the code is in the head section of the html then the first time you call the display() the timeframe element has not been created yet..

instead of display(); use window.onload=display; to run it for the first time, after the dom has loaded..

here is a fixed working version of your code.. http://jsbin.com/orege/edit

Improve this answer

Comments

0

One thing that jumps out at me is that your calculation at the top (starting with var temp...) should be inside the display function (which probably wants a new name). Where it is, it's only done once. (I'm also not convinced that the actual calcuation is correct. In fact, the code overall is not really ideal, you might not want to get more code from wherever you got this. But I'm not going to rewrite it for this answer.)

See also n00b32's answer, there seems to be a syntax error near the top.

Also (OT): You can change your setTimeout to: setTimeout(display, 1000). No need for the quotes and parens.

Improve this answer

Comments

0

As an additional improvement (but first get the basic version working by reading everyone else's comments), I like to replace this line

   setTimeout(display, 1000);

with this:

   var next = 1010 - (new Date()).getTime() % 1000;
   setTimeout(display, next);

The first version will never run exactly every second (due to other processes running and other inaccuracies in setTimeout). This can make your display jump 2 seconds at once when one call of display() happens at the end of second n and the next call only happens at the beginning of n+2. The second version looks at the current time (down to the millisecond), and tries to schedule the next call of display() just after the start of each second, which results in a much smoother display.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.