About a loop that creates dynamic buttons, but cannot give proper values

Question

Possible Duplicate:
Javascript infamous Loop problem?

I am having a small issue, and it would be very nice if some of you could realize about what kind of logic is missing here, since I cannot seem to find it:

I have an array with the results of some previous operation. Let's say that the array is:

var results = [0, 1];

And then I have a bunch of code where I create some buttons, and inside a for loop I assign a different function to those buttons, depending on the position of the array. The problem is that for some reason, all the buttons created (two in this case) come out with the function assigned to the last value of the array (in this case, both would come out as one, instead of the first with 0 and the second with 1)

This is the code:

for (var i = 0; i < results.length; i++) {
    var br2 = b.document.createElement("br");
    var reslabel = b.document.createTextNode(Nom[results[i]].toString());
    var card = document.createElement("input");
    card.type = "button";
    id = results[i]; // this is the problematic value. 
    card.onclick = newcard; // this function will use the above value.
    card.value = "Show card";
    divcontainer.appendChild(br2);
    divcontainer.appendChild(reslabel);
    divcontainer.appendChild(card);
}

As it is, this code produces as many buttons as elements in the array, each with its proper label (it retrieves labels from another array). Everything is totally fine. Then, I click the button. All the buttons should run the newcard function. That function needs the id variable, so in this case it should be:

First button: runs newcard using variable id with value 0
Second button: runs newcard using variable id with value 1

But both buttons run using id as 1... why is that?

It might be very simple, or maybe is just that in my timezone is pretty late already :-) Anyways, I would appreciate any comment. I am learning a lot around here...

Thanks!

Edit to add the definition of newcard:

function newcard() {
    id = id;
    var toerase = window.document.getElementById("oldcard");
    toerase.innerHTML = "";
    generate();
}

the function generate will generate some content using id. Nothing wrong with it, it generates the content fine, is just that id is always set to the last item in the array.

You don't actually use `id` inside of `newcard` (`id = id` does nothing). Do you use it in `generate`? — bfavaretto, Dec 22 '12 at 00:52
yes, I need the `id` value to be defined, because `generate` will ask for it. — telex-wap, Dec 22 '12 at 00:58
This is not an *exact* duplicate of the linked question, despite the fact that I initially thought it was, and voted to close. Voted to reopen. — bfavaretto, Dec 23 '12 at 15:05

RonaldBarzell · Answer 1 · 2012-12-22T01:44:12.450

3

Before going on, a HUGE thank you to bfavaretto for explaining some scoping subtelties that totally escaped me. It seems that in addition to the problems you had, you were also suffering from scoping, which bit me while I was trying to craft an answer.

Anyway, here's an example that works. I'm using forEach, which may not be supported on some browsers. However it does get around some of the scoping nastiness that was giving you grief:

<html>
<body>
<script>
var results = [0,1];
results.forEach( function(result) {
    var card = document.createElement("input");
    card.type = "button";
    card.onclick = function() {
        newcard( result );
    } 
    card.value = "Show card";
    document.body.appendChild(card);
}); 

function newcard(x) {
   alert(x);
}
</script>
</body>
</html>

If you decide to stick with a traditional loop, please see bfavaretto's answer.

edited Dec 22 '12 at 01:44

answered Dec 22 '12 at 00:41

RonaldBarzell

3,822
1
16
23

very well explained, and I saw some things I was doing wrong thanks to your explanation. Still, the problem remains. I have done exactly like that: changing the onclick assignment and changing the definition of newcard. But I put a `console.log(id)` inside the function of the onclick, and it's always the last value... I will stop now, and tomorrow I will re-read your answer and keep working on it. – telex-wap Dec 22 '12 at 00:57
@RonaldBarzell That won't work, because (1) `id` is global, and (2) it is being used in `generate`. The latter would have to be passed an id too. And (3) looks like you forgot to create the actual closure, you'd need something like `card.onclick = (function(id) { return function() { newcard(id); }; }(results[i]))`. – bfavaretto Dec 22 '12 at 01:01
@bfavaretto: Actually, the code you showed won't work as you're assigning a function of one parameter to onclick, which expects a function of no parameters. However, the code I displayed is broken, which is odd, as I've written code like that dozens of times with no problem. Looking at it further to see what I missed... – RonaldBarzell Dec 22 '12 at 01:08
See my answer (formatting is better there). I'm actually assigning a function with no parameters as the event handler (which is actually passed one parameter, the event object, which I left out). – bfavaretto Dec 22 '12 at 01:10
@bfavaretto Actually, I modified my answer with full code that does work. It's a loop oddity... – RonaldBarzell Dec 22 '12 at 01:13
It works now because with `forEach` you're actually creating a closure - plus you got rid of the global `id` variable. – bfavaretto Dec 22 '12 at 01:16
@bfavaretto Ok, I'm completely lost. Why was I creating a closure with forEach but not with for, and why did id being global have anything to do with it? I thought closures captured free variables and that was that? – RonaldBarzell Dec 22 '12 at 01:19
let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/21549/discussion-between-bfavaretto-and-ronaldbarzell) – bfavaretto Dec 22 '12 at 01:20

score 3 · Accepted Answer · edited May 23 '17 at 12:18

Your id is a global variable, and when the loop ends it is set to the last value on the array. When the event handler code runs and asks for the value of id, it will get that last value.

You need to create a closure to capture the current results[i] and pass it along (this is a very common pitfal, see Javascript infamous Loop problem?). Since newcard is very simple, and id is actually used in generate, you could modify generate to take the id as a parameter. Then you won't need newcard anymore, you can do this instead:

card.onclick = (function(id) { 
    return function() { 
        window.document.getElementById("oldcard").innerHTML = "";
        generate(id);
    }; 
}(results[i]));

What this does is define and immediately invoke a function that is passed the current results[i]. It returns another function, which will be your actual onclick handler. That function has access to the id parameter of the outer function (that's called a closure). On each iteration of the loop, a new closure will be created, trapping each separate id for its own use.

Thanks, I understand the problem and your solution, although I cannot really use it 100% because I cannot set `generate` to take a parameter (due to other parts of the program needing it like this), but with your explanation as starting point, I will see what can I do. — telex-wap, Dec 22 '12 at 10:15
Well, I got it to work with minimal changes to your example. Excellent! This was really something... I will bookmark this answer for future reference. — telex-wap, Dec 22 '12 at 10:51

About a loop that creates dynamic buttons, but cannot give proper values

2 Answers2