16

I'm relatively new to JS so this may be a common problem, but I noticed something strange when dealing with for loops and the onclick function. I was able to replicate the problem with this code: 

<html>
<head>

<script type="text/javascript">
window.onload = function () {
    var buttons = document.getElementsByTagName('a');
    for (var i=0; i<2; i++) {
        buttons[i].onclick = function () {
            alert(i);
            return false;
        }
    }
}

</script>

</head>

<body>
<a href="">hi</a>
<br />
<a href="">bye</a>

</body>

</html>

When clicking the links I would expect to get '0' and '1', but instead I get '2' for both of them. Why is this?

BTW, I managed to solve my particular problem by using the 'this' keyword, but I'm still curious as to what is behind this behavior.

pythonBOI
  • 163
  • 1
  • 4

3 Answers3

16

You are having a very common closure problem in the for loop.

Variables enclosed in a closure share the same single environment, so by the time the onclick callback is executed, the loop has run its course and the i variable will be left pointing to the last entry.

You can solve this with even more closures, using a function factory:

function makeOnClickCallback(i) {  
   return function() {  
      alert(i);
      return false;
   };  
} 

var i;

for (i = 0; i < 2; i++) {
    buttons[i].onclick = makeOnClickCallback(i);
}

This can be quite a tricky topic, if you are not familiar with how closures work. You may to check out the following Mozilla article for a brief introduction:

Note: I would also suggest not to use var inside the for loop, because this may trick you in believing that the i variable has block scope, when on the other hand the i variable is just like the buttons variable, scoped within the function.

Daniel Vassallo
  • 337,827
  • 72
  • 505
  • 443
9

You need to store the state of the i variable, because by the time the event fires, the scoped state of i has increased to the maximum loop count.

window.onload = function () {
    var buttons = document.getElementsByTagName('a');
    for (var i=0; i<2; i++) {
        (function (i) {
            buttons[i].onclick = function () {
                alert(i);
                return false;
            }
        })(i);
    }
}

The above example creates an anonymous function with a single argument i, which is then called with i being passed as that argument. This creates a new variable in a separate scope, saving the value as it was at the time of that particular iteration.

Andy E
  • 338,112
  • 86
  • 474
  • 445
  • 1
    Creating functions in a loop is not recommended – Ryan Kinal May 06 '14 at 02:07
  • @Ryan: I'm not sure if that's pedantry or confusion talking. You're repeating a JSLint/JSHint warning without taking the time to understand what it means. Take a look at the [docs](http://www.jshint.com/docs/options/#loopfunc) for an example of a function that shouldn't be created in a loop. IIFEs are acceptable because it's pretty much the only option in some scenarios until `let` becomes universally supported. The accepted answer creates functions in loops too, no down vote or comment there, though? – Andy E May 06 '14 at 07:05
  • Okay, let me clarify: You're creating *two* functions in a loop. The IIFE, and the click handler. The accepted answer is creating one function in a loop: the click handler. Thus, the accepted answer minimizes the inefficiencies, while yours maximizes them. – Ryan Kinal May 06 '14 at 14:51
  • @Ryan: so you *were* being pedantic then, because you down voted over a micro optimisation. And not even a real one, at that, because it fails to consider the compiler's own optimisations. In Chrome's console, the IIFE comes out at ~100ms faster over 1 million operations (I encourage you to check it). Not that it even matters, because the question is looping from `i` = `0` to `2`, so the difference will be immeasurable. Congratulations, you're the traffic officer who gives the guy a parking fine for arriving back at his car 1 second after his ticket expires. – Andy E May 06 '14 at 15:52
  • *:sigh:* Yes, I guess I was being pedantic. So sue me. Go take your perfs and your compilers and your "I'm right and I have 126k rep" and prance around your perfect little world with all rightness and perfiness of everything you ever wanted. – Ryan Kinal May 06 '14 at 16:23
0

It's a order of execution issue

How to assign event callbacks iterating an array in javascript (jQuery)

Basically, the click handler accesses i well after the loop has exited, and therefore i is equal to 2.

Community
  • 1
  • 1
Peter Bailey
  • 105,256
  • 31
  • 182
  • 206