0 
 var RelevantDiv = document.getElementById("RelevantDiv");
 for (var i = 0; i < 5; i++) {
    for (var j = 0; j < 5; j++) {
       var NewButton = document.createElement("NewButton");
       var IdString = "ID" + i + "_"  +j;
       NewButton.onclick = function() { DoStuff(IdString) };
       RelevantDiv.appendChild(NewButton);
    }
 } 

 function DoStuff(IdForStuff) {
    // Does stuff with id 
 }

The problem is that every time the NewButton.onclick is set, that it is setting it to the final IdString which is "ID4_4".

Improve this question
2
  • What environment are you running in? Is ES2015 available? A simple switch from var to let would help if so. Commented Aug 9, 2016 at 15:34
  • @JamesThorpe needs to work for as many browsers, versions as possible. Commented Aug 9, 2016 at 15:36

3 Answers 3

Reset to default
2

Worth noting, that you won't have such problems if you use let or constinstead of var:

for (let i = 0; i < 5; i++) {
    for (let j = 0; j < 5; j++) {
       const NewButton = document.createElement("NewButton");
       const IdString = "ID" + i + "_"  +j;
       NewButton.onclick = function() { DoStuff(IdString) };
       RelevantDiv.appendChild(NewButton);
    }
}
Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

And still, every iteration it will be overwritten, or am I missing something?
Well, if you replace var with let or const which are not hoisted, the problems disappears.
1

Your underlying issue is that the variable being used within the onclick callback, IdString, is having its declaration hoisted to the top of the current scope, ie the function it's running within. That means that every time you're within the loop, its value is overwritten - it's functionally the same as this:

var IdString;

var RelevantDiv = document.getElementById("RelevantDiv");
for (var i = 0; i < 5; i++) {
   for (var j = 0; j < 5; j++) {
      var NewButton = document.createElement("NewButton");
      IdString = "ID" + i + "_"  +j;
      NewButton.onclick = function() { DoStuff(IdString) };
      RelevantDiv.appendChild(NewButton);
   }
}

You need to ensure that you capture the right value of IdString when you need it, which is typically done through the use of a closure:

var RelevantDiv = document.getElementById("RelevantDiv");
for (var i = 0; i < 5; i++) {
   for (var j = 0; j < 5; j++) {
      (function(IdString) {
          var NewButton = document.createElement("NewButton");
          NewButton.onclick = function() { DoStuff(IdString) };
          RelevantDiv.appendChild(NewButton);
      })("ID" + i + "_"  +j)
   }
}

Here we create another inner function to create a new scope to hold each individual IdString value. It's then immediately called with the right value for each iteration of the loops. IdString is then captured within this closure, and the correct value will be used within the onclick callback.

Alternatively, you can bind the argument at the moment you know what the relevant value is:

var RelevantDiv = document.getElementById("RelevantDiv");
for (var i = 0; i < 5; i++) {
   for (var j = 0; j < 5; j++) {
      var NewButton = document.createElement("NewButton");
      NewButton.onclick = DoStuff.bind(null, "ID" + i + "_"  +j);
      RelevantDiv.appendChild(NewButton);
   }
}

This does away with both of the inner functions, and directly assigns the onclick event to a copy of the DoStuff function that will have the right argument.

Improve this answer

Comments

1

The problem here is that, when the handler you assign runs, the loop has already looped through all iterations, and the variable at that point will be at its final state. To avoid this, you need to use a lock. It involves a self-executing function (function () { // code })() which saves the current state of the variable. When the variable in the current iteration, for example State 2 is given as an argument to the self-executing function, the scope of the function will save this state. When the for loop continues, the original variable will still change, but the scope of the new "inner function" will keep the value "locked". So even when the loop-variable will in the end be at its 4th state, the locked variable will still be in state 2.

for (var i = 0; i < 5; i++) {
    button.addEventListener ("click", function () {
        // Here, i would normally always be 5
        (function (lockedIndex) {
            // This lock will make i the way it should be
            // You have to use lockedIndex instead of i
        })(i)
    });
}

For your code, this would look something like:

 var RelevantDiv = document.getElementById("RelevantDiv");
 for (var i = 0; i < 5; i++) {
    for (var j = 0; j < 5; j++) {
        (function (ix, jx) {
          var NewButton = document.createElement("NewButton");
          var IdString = "ID" + ix + "_"  +jx;
          NewButton.onclick = function() { DoStuff(IdString) };
          RelevantDiv.appendChild(NewButton);
        })(i, j)
    }
 } 

 function DoStuff(IdForStuff) {
    // Does stuff with id 
 }
Improve this answer

7 Comments

i isn't the direct issue here, it's the fact that IdString is being hoisted.
Then, use the exact same method for IdString. I've just used i as example, it does not reference the ì` in your code.
Indeed, but your answer could be more detailed about the problem.
The problem isn't hoisting, it is the way values are shared with closures. @NikxDa solution solves this by passing the value on each iteration.
@NikxDa can you make this solution work for the string requested.
|

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.