Incorrectly passing callback parameter inside a for loop

Question

I have a variable id that is generate in a for loop. For some reason, the id changes from 0 to 1 when it is passed in a callback. My guess is that I am passing the id parameter incorrectly.

    refreshMenu: function(mods, callback){
    var menu = document.getElementById('menu'), 
            ul = document.createElement('ul');

    menu.innerHTML = ''; 
    menu.appendChild(ul);

    for(var id = 0; id < mods.length; id++){
        var li = document.createElement('li'), 
        that = this;


            ul.appendChild(li);

            li.setAttribute('id', id);
            li.innerHTML = mods[id].get('name');

            console.log(id); // HERE ---> 0

            li.addEventListener('click', function(){
                that.selectMenu(li, function(){

                    console.log(id); HERE ---> //1 ?

                that.selectList(id - 1);

                });
            }, false);
        }

        callback();
    }, 

    selectList: function(id)
        var activeList = this.activeList();

        console.log(id);

        if (activeList.id == id){
            return;
        } 
        else if (activeList){
            activeList.setAttribute('class', '');
        };

        var target = document.getElementById(id);
        target.setAttribute('class', 'activeList');
    },  

    selectMenu: function(li, callback){
        if(li.className == 'activeMenu') 
            return; 

        var active = document.getElementsByClassName('activeMenu')[0];
        if(active)      
            active.setAttribute('class', '');

        li.setAttribute('class', 'activeMenu');

        callback();
    },

Please see: http://stackoverflow.com/questions/3572480/please-explain-the-use-of-javascript-closures-in-loops/3572616#3572616 — slebetman, Sep 24 '12 at 02:54

dyoo · Accepted Answer · 2012-09-24T03:40:29.263

3

It's a very common gotcha when building closures within for loops. All of the functions you're creating refer to the same id variable, and the for loop mutates the variable.

One simple way to fix this: make a helper function that creates these functions, and have the helper take its own parameter.

// In the body of the for loop ...
var makeClickFunction = function (li, id) {
    return function (){
        that.selectMenu(li, function(){
                               console.log(id);
                               that.selectList(id - 1);
                            });
    };
};
li.addEventListener('click', makeClickFunction(li, id));

The helper's binding of its id argument shadows that of the for loop, and this is a good thing! We're intentionally doing this. We also capture li, since it too needs to be held.

Later, we call makeClickFunction, passing in the current value within the for loop. for will continue to mutate its id, but that's ok: that mutation has no relationship to the behavior on makeClickFunction's closures.

edited Sep 24 '12 at 03:40

answered Sep 24 '12 at 02:53

dyoo

11,795
1
34
44

1

Note that you also need to pass `li` and `that` into the helper function. – slebetman Sep 24 '12 at 02:55
1

thanks. this makes much more sense. you forgot the closing brackers for `that.selectMenu` though. i.e. `});` – Sep 24 '12 at 03:22
Thanks to both slebetman and user155300; the answer should be corrected now. I don't believe *that* has to be re-captured by the closure; it appears to stay constant (or at the very least, reassigned to the same value) throughout the loop. – dyoo Sep 24 '12 at 03:41
@dyoo: It depends on where you declare the `makeClickFunction`. If you declare it within the same closure then `that` does not need to be passed. But if it is declared elsewhere outside of the closure then you need to pass in `that`. I was assuming it was declared elsewhere. Missed the comment about it being declared in the loop. – slebetman Sep 24 '12 at 04:20

Marcus · Answer 2 · 2012-09-24T02:58:30.483

1

The value of id has been already changed before the callback is called, so if you want the correct id value when the callback being called, you need the closure.

        li.addEventListener('click', function(){
            that.selectMenu(li, function(){

                console.log(id); HERE ---> //1 ?

            that.selectList(id - 1);

            });
        }, false);

==>

li.addEventListener('click', 
(function(li,that,id){
    return function(){
        that.selectMenu(li, function(){
        console.log(id); 
        that.selectList(id - 1);
        })
    }
})(li,that,id),
false);

edited Sep 24 '12 at 02:58

answered Sep 24 '12 at 02:28

Marcus

6,701
4
19
28

it says `undefined is not a function`. Also you forgot 1 ending bracket for `addEventListener`. – Sep 24 '12 at 02:45
That's not really creating a closure yet. It should end in `})(li,this,id));` though to be clearer I'd prefer if the inner variables have different names so that it's clear that you are trying to break the closure. – slebetman Sep 24 '12 at 02:51
Also the explanation is wrong. It should be `The variable id is shared by the callbacks because it is captured in a closure, so if you want the correct id in the callback, you need to break the closure.` – slebetman Sep 24 '12 at 02:52
@user1555300, the `id` is changed by the `id++`, the `id` that you use in the loop and the callbacks refer to a same object if you don't use closure. – Marcus Sep 24 '12 at 03:00

Incorrectly passing callback parameter inside a for loop

2 Answers2