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.

Improve this question
1
  • Please show an example of how you solved the problem with "this." Commented Feb 17, 2017 at 19:05

3 Answers 3

Reset to default
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.

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

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.

Improve this answer

5 Comments

Creating functions in a loop is not recommended
@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 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?
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: 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.
: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.
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.

Improve this answer

2 Comments

It is also a scope issue, i is hanging around when it is not apparent it should.
Right, which is why I linked to a post of mine about closures.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.