2

I have a javascript code like this and this always gives me a problem

    for(var i=1;i<9;i++){
        document.getElementById('element'+i).onclick=function(){
             theFunc(i)
        }
    }

It selects the right element and adds the onclick. But, when I type in console document.getElementById('element1").onclick it returns theFunc(i) (not theFunc(1))

So no matter which element is clicked it will always call theFunc(9) (at the end i is 9)

What's wrong with my code?

AMD
  • 1,278
  • 4
  • 15
  • 33

3 Answers3

5

Your event handler function has an enduring reference to i, not a copy of its value, as you've discovered.

To prevent that, have the function close over something else that won't change:

for(var i=1;i<9;i++){
    document.getElementById('element'+i).onclick=makeHandler(i);
}

function makeHandler(index) {
    return function() {
        theFunc(index);
    };
}

makeHandler creates a function that closes over index, which is a copy of the value of i, and so doesn't change as the loop continues. Each event handler gets its own index.

That said, creating a bunch of event handler functions that are effectively identical usually means you can redesign a bit and use just one handler function. In this case, for instance, you could do this:

for(var i=1;i<9;i++){
    document.getElementById('element'+i).onclick=theHandler;
}

function theHandler() {
    func(parseInt(this.id.replace(/\D/g, ''));
}

...which grabs the value to use from the id of the element.

Another approach is delegation, where you actually hook the click event on an ancestor element (one that all of these elementX's have in common), and then when the click occurs, look at event.target and its ancestors to see what you should do.

T.J. Crowder
  • 1,031,962
  • 187
  • 1,923
  • 1,875
  • You are great T.J. Crowder – AMD Aug 06 '13 at 14:53
  • Although it's limited in support and messed up the order of parameters, you could also use `.bind()`: http://jsfiddle.net/Cs8Mb/ . I'm not sure I'd recommend it for this situation, but it works – Ian Aug 06 '13 at 15:27
1

TJ Crowder's answer is the best way around your problem. This "problem" you're experiencing in your closure is by design in many languages, and is referred to as scope.

Here's a good explanation of different scopes in JavaScript (including closures) and how to use them. http://robertnyman.com/2008/10/09/explaining-javascript-scope-and-closures/

chris.nesbit1
  • 333
  • 2
  • 9
0

When you say theFunc(i) you are creating a closure around i, such that every function call refers to the same variable. You need to wrap the function inside an outer closure to ensure each function call is working with a unique variable:

for(var i=1;i<9;i++){
  (function(i){

    document.getElementById('element'+i).onclick=function(){
         theFunc(i);
    }

  })(i);
}
Justin Ethier
  • 131,333
  • 52
  • 229
  • 284
  • 2
    I can't see why this is downvoted. However, note that creating a new *builder* function on every loop iteration is not best practice (you have to create a new event handler, but you're actually creating two functions on each loop, the builder function and the event handler function.) Also, I always recommend using a *different* name for the argument, for clarity. – T.J. Crowder Aug 06 '13 at 14:42
  • @T.J.Crowder I think you just explained why it deserves a downvote. This also suffers from the fact that it closes over all other local variables, not allowing them to be GC'ed. Your answer doesn't allow that, which is great. – Ian Aug 06 '13 at 15:23
  • @Ian - Assuming there are other local variables, of course... – Justin Ethier Aug 06 '13 at 15:27
  • @JustinEthier Of course, I was just pointing it out :) – Ian Aug 06 '13 at 15:28