Simple javascript not working

Discussion in 'JavaScript' started by aab1, Feb 22, 2014.

  1. #1
    I programmed Javascript 15 years ago and almost haven't used it since, I mostly use VB/ASP now.

    I have a simple countdown I want to have on my website and I can't even get something that simple working, I want it to go down by 1 second every second, this is my code that doesn't work (it loads the initial time but does not decrease it by one second per second):

    <script language="JavaScript">
    var UserSaleTimeLeft = <some large number of seconds added here by ASP>
    
    function UpdTime(){
    UserSaleTimeLeft=UserSaleTimeLeft-1
    var HoursLeft = Math.floor(UserSaleTimeLeft / 3600)
    var MinutesLeft = Math.floor((UserSaleTimeLeft - (HoursLeft * 3600)) / 60)
    var SecondsLeft = UserSaleTimeLeft - (HoursLeft * 3600) - (MinutesLeft * 60)
    document.getElementById('SaleTimeLeft').innerHTML = HoursLeft + " hours, " + MinutesLeft + " minutes, " + SecondsLeft + " seconds";
    setTimeout(UpdTime(),1000)}
    
    UpdTime();
    
    </script>
    
    Code (markup):
    The problem is likely either:
    1. The "UserSaleTimeLeft=UserSaleTimeLeft-1" is not subtracting 1 to the initial value, I remember about some "--" or "=--" thing to decrease by 1 but don't remember exactly what it is

    2. The setTimeout is not executing every second.

    What did I do wrong?

    Thanks
     
    aab1, Feb 22, 2014 IP
  2. xtmx

    xtmx Active Member

    Messages:
    359
    Likes Received:
    12
    Best Answers:
    4
    Trophy Points:
    88
    #2
    setTimeout(UpdTime(),1000);

    should be: setTimeout(UpdTime,1000);

    setTimeout takes a reference to the function to execute, or executes a quoted string of javascript code (you are generally supposed to use function references, though)
     
    xtmx, Feb 24, 2014 IP
  3. deathshadow

    deathshadow Acclaimed Member

    Messages:
    9,732
    Likes Received:
    1,999
    Best Answers:
    253
    Trophy Points:
    515
    #3
    xtmx has it right in that you have to pass just the function name, adding () makes the function run right there, so what's passed to setTimeout would be whatever your function returns, not the function itself.

    That said... fifteen years? Looks about right, that's the last time the 'language' attribute was valid; in a current recommendation document you should be using type="text/javascript" instead, or in the bleeding edge draft known as 5, carefully crafted to set coding practices back to the last time you used JS, you omit both.

    Your math could be greatly simplified using a 'modulo' -- the % sign is the modulo operator... what's a modulo you ask? It's a big fancy goofy name for... the remainder. Remember short division with remainders from grade school? (if you were doing JS a decade and a half ago you should be old enough to remember that as opposed to the inept useless gibberish they teach today)... you've got some big long math in there for little reason.

    That means to figure out seconds it's just time % 60, for minutes it's Math.floor(time/60) % 60, and for hours just floor /3600

    Likewise I'd probably axe the 'variables for nothing' -- if you're only using a result once, calculate it when used rather than wasting time on a variable.

    InnerHTML also forces a reflow, which is slow and CPU hungry. These days even though it's a wee bit more code, we're supposed to be using the DOM to add/remove content.

    I would probably also make the function for those an object, allowing it to be instatiated multiple times so you can have more than one timer if desired, and to keep the actual working bits out of the global namescape. You could pass the ID and time to the function, letting you move that script out of the markup into an external file where cache can be leveraged.

    So for example if you had:
    <div id="SaleTimeLeft1"></div>
    <div id="SaleTimeLeft2"></div>
    Code (markup):
    This scripting would be more along the lines of what I'd use.

    <script type="text/javascript"><!--
    
    function startTimer(displayId, period) {
    
    	var
    		timer = period,
    		display = document.getElementById(displayId);
    
    	this.update = function() {
    		if (--timer == 0) clearInterval(intHandler);
    		while (display.firstChild) display.removeChild(display.firstChild);
    		display.appendChild(document.createTextNode(
    			Math.floor(timer / 3600) + ' hours, ' +
    			Math.floor(timer / 60) % 60 + ' minutes, ' +
    			timer % 60 + ' seconds'
    		));
    	}
    	
    	var intHandler = setInterval(this.update,1000);
    
    }
    
    startTimer('SaleTimeLeft1', 3760);
    startTimer('SaleTimeLeft2', 1808);
    
    --></script>
    Code (markup):
    Notice I also kill the update when the value hits zero, so it doesn't keep counting past the timeout. Fun thing about functions, if you set a value that isn't released, each call is treated as a new object; this is why rogue/poorly written scripts can suck memory dry and add CPU time to suck batteries dry too.
     
    deathshadow, Feb 25, 2014 IP