multiple setTimeout calls within a for loop

Question

I'm trying to code the game Simon in HTML/JS and it's all working, except for the part where the game flashes the sequence so you know what the new sequence is. Essentially what I have is:

for(var i in thePattern){
    var obj = document.getElementById(thePattern[i]);
    window.setTimeout(colorON(obj),500);
    window.setTimeout(colorOFF(obj),1000);
}

where colorON and colorOFF are:

function colorON(obj){
    if(obj.id == "red"){
        obj.style.backgroundColor="#ff5555";
    }else if(obj.id == "blue"){
    obj.style.backgroundColor="#5555ff";
    }else if(obj.id == "green"){
    obj.style.backgroundColor="#88ff88";
    }else{
    obj.style.backgroundColor="#ffffaa";
    }
}
function colorOFF(obj){
    if(obj.id == "red"){
        obj.style.backgroundColor="#ff0000";
    }else if(obj.id == "blue"){
        obj.style.backgroundColor="#0000ff";
    }else if(obj.id == "green"){
        obj.style.backgroundColor="#22ff22";
    }else{
        obj.style.backgroundColor="#ffff00";
    }
}

What it seems to be doing is going through the entire for loop and starting all the timers and then then all the timers go off so quickly that the colors don't even seem to flash.

Any ideas? All help is greatly appreciated.

Now it is flashing correctly and the closure is working correctly, but it is flashing all the colors at the same time. I've tried putting the closure within another setTimeout, however, that just creates other problems.

SOLVED thanks for your help guys.

First fix the problems pointed out in the answers here. Then when you find the next issue that fixing this will expose, have a look at this: http://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example — James Montagne, May 13 '13 at 17:36

Blender · Accepted Answer · 2013-05-13T17:38:08.060

7

You need to pass a function to setTimeout:

window.setTimeout(function() {
    colorON(obj);
},500);

Right now, you're calling colorON(obj) immediately and passing its output to setTimeout, which makes it appear like setTimeout is firing immediately.

obj is also passed by reference, so by the time all of your functions will run, obj will refer to the last element in your loop. To get around that, you have to pass obj by value by shadowing it:

(function(obj) {
    window.setTimeout(function() {
        colorON(obj);
    }, 500);

    window.setTimeout(function() {
        colorOFF(obj);
    }, 1000);
})(obj);

edited May 13 '13 at 17:38

answered May 13 '13 at 17:32

Blender

289,723
53
439
496

You'll hit the *"creating a function in a loop"* problem. OP should keep the same code, and have the functions return a function. – May 13 '13 at 17:33
1

This is true, but fixing it will cause another issue. Calling `setTimeout` in a loop, the `obj` value will always be the final value of the last iteration. – James Montagne May 13 '13 at 17:34
1

@squint: I'd rather just shadow `obj`. Having `colorON` return a function is counter-intuitive. – Blender May 13 '13 at 17:39
There's nothing intuitive about IIFE's... evidenced by all the people that ask about them... but ultimately the only "intuintive" solution is for JS to have block scope... again, evidenced by the fact that so many people expect it. – May 13 '13 at 17:43
@squint: That's what the `let` keyword is for. Sadly, nobody supports it. – Blender May 13 '13 at 18:14
Well... almost nobody, but certainly not enough to be able to use it. :( – May 13 '13 at 18:55
@Blender regarding let keyword: It's true its support isn't widespread yet, but you can use the with keyword to similar effect. EDIT: Googled and found this from SO: http://stackoverflow.com/a/185283/681290 – Tyler Biscoe May 13 '13 at 19:59
Could anybody explain little bit more this application of variable shadowing please? Everywhere it's explained on basic concept of local vs. global variable. But the code above is quite difficult to understand for me. Especially the purpose of `(obj)` at the end. Thanks! – Enriqe Sep 01 '14 at 11:02

epascarello · Answer 2 · 2013-05-13T17:50:00.973

You are calling the function, not assigning a reference to it! So the code runs right away and sets the setTimeout with whatever the function returns.

change

window.setTimeout(colorON(obj),500);
window.setTimeout(colorOFF(obj),1000);

to

for(var i in thePattern){
    var obj = document.getElementById(thePattern[i]);
    (function(obj) {
        window.setTimeout(function(){colorON(obj);},500);
        window.setTimeout(function(){colorOFF(obj);},1000);
    })(obj);
}

and code showing you how to do with with a switch or an object to get rid of the if/else logic

function colorON(obj) {
    var color = "";
    switch (obj.id) {
        case "red":
            color = "#ff5555"
            break;
        case "blue":
            color = "#5555ff"
            break;
        case "green":
            color = "#88ff88"
            break;
        default:
            color = "#ffffaa"
    }
    obj.style.background = color;
}

var colorsOff = {
    "red": "#ff0000",
        "blue": "#0000ff",
        "green": "#22ff22",
        "default": "#ffff00"
}



    function colorOFF(obj) {
        var color = colorsOff[obj.id] || colors["default"];
        obj.style.backgroundColor = color;
    }


var thePattern = {
    "one": "red",
        "two": "blue",
        "three": "green"
}


for (var i in thePattern) {
    var obj = document.getElementById(thePattern[i]);
    (function (obj) {
        window.setTimeout(function () {
            colorON(obj);
        }, 500);
        window.setTimeout(function () {
            colorOFF(obj);
        }, 1000);
    })(obj);
}

Example: http://jsfiddle.net/brjgc/

You'll hit the *"creating a function in a loop"* problem. OP should keep the same code, and have the functions return a function. — , May 13 '13 at 17:34

multiple setTimeout calls within a for loop

2 Answers2