6

I have a some buttons, which are stored in an array. I then loop through that array to add a click event to each button. Each click alerts the value of i. I expect the values to be 1, 2, 3 and so on, but they always come back as one value, in case 3.

Can you explain why this happens and how to fix it?

Please see this a jsFiddle. Code below:

var theButtons = ['.button.one', '.button.two', '.button.three'];

for (i=0; i<theButtons.length; i++) {
    $(theButtons[i]).click(function () {
        alert(i); // always returns 3
    });
}

Please explain it as simply and clearly as you can - I'm somewhat of a beginner at Javascript and programming.

j08691
  • 204,283
  • 31
  • 260
  • 272
shrewdbeans
  • 11,971
  • 23
  • 69
  • 115
  • 1
    Start reading at http://stackoverflow.com/questions/2568966/how-do-i-pass-the-value-not-the-reference-of-a-js-variable-to-a-function – DCoder Jul 19 '13 at 15:43
  • 2
    why do questions that have been asked a thousand times get upvoted? – Kevin B Jul 19 '13 at 15:46
  • I would say you don't want to code those events in that way anyway... – Alvaro Jul 19 '13 at 15:47
  • @Alvaro You should explain **why** he shouldn't instead of leaving this empty comment there. – Jeff Noel Jul 19 '13 at 15:49
  • As a programming and especially a javascript beginner, you may want to have a few drinks before you start reading about closures. Your brain will hurt for awhile until you start to understand it. – Joe Enos Jul 19 '13 at 15:49
  • @shrewdbeans Does this work for you? http://jsfiddle.net/fE55Y/6/ – Iron3eagle Jul 19 '13 at 15:49
  • Wow this question has the most amount of answer with down votes I've ever seen... – Iron3eagle Jul 19 '13 at 15:51

3 Answers3

11

By the time you click on the button i === 3. Need to pass the value of i into a closure:

for (var i = 0; i<theButtons.length; i++) { // do `var i` so the i is not global
    (function(index){
        $(theButtons[i]).on('click', function () {
           alert(index); // index === the value that was passed
        });
    })(i); // pass the value of i
}

Fiddle Demo: http://jsfiddle.net/maniator/fE55Y/3/

Naftali
  • 144,921
  • 39
  • 244
  • 303
2

Just use EACH method:

$( ".button" ).each(function( index ) {
    $(this).click( function(e){
        alert( index + ": " + $(this).text());
    });
});

Fiddle: http://jsfiddle.net/fE55Y/4/

Update:

Agreed that .each() is not needed for what's been asked. Here's the one without using .each() method.

$(".button").click(function () {
    alert("Button Index: " + $(this).index());
});
Learner
  • 3,904
  • 6
  • 29
  • 44
  • 3
    @Neal - well if jQuery is in use anyway using `.each()` isn't adding to any bloat. – j08691 Jul 19 '13 at 15:51
  • @j08691 This answer is relying on the DOM element's text -- which is useless? Also the op is not ness going through **all** buttons, but he is only going through a set array of buttons... – Naftali Jul 19 '13 at 15:51
  • 4
    He also has his question tagged.. and is using jQuery already. Sure you can do it in pure JS, but theres no reason for this to be downvoted. – Loktar Jul 19 '13 at 15:52
  • it is adding bloat to the processing... not that it would be noticed, but Neals comment is still kinda valid. however his use of the .click alias is equally as silly. – rlemon Jul 19 '13 at 15:52
  • 1
    The problem with this answer is that it's not explaining the problem in th original code, which is at least half of the actual question. – bfavaretto Jul 19 '13 at 15:53
  • 1
    @Neal - Oh I'm not saying that this is the right answer, I'm just saying that if the OP is using jQuery that I don't see where the bloat came from. If the question didn't use jQuery and this answer suggested it then sure, unnecessary extra code for something that basic. – j08691 Jul 19 '13 at 15:54
  • Neal, please explain how is it bloated and if it was, why did jQuery developers build such method? And my main point is if you want to write function in pure JS style what's the need of jQuery? – Learner Jul 19 '13 at 15:56
  • @Learner That is why there is a normal `.forEach` loop on an array. There is no real reason to use `$.each` especially since it did not even answer the OPs question. – Naftali Jul 19 '13 at 15:57
  • @j08691 ^^ see above. – Naftali Jul 19 '13 at 15:58
  • 1
    @Neal the questioner has a tag "jQuery" and .foreach is a pure JS function and not jQuery. I saw the tag and provided him the jQuery solution. As for second part "that i did not answer what's been asked" it's a totally different thing and shouldn't be mixed up with $.each conversation. – Learner Jul 19 '13 at 16:02
  • @Learner and the `$(this)` selects over and over gain? Cache them! – Naftali Jul 19 '13 at 16:02
  • Another thing I have been noticing on SO is that some people just Down-Vote without understanding, because someone with higher VOTES is not convinced with your solution. – Learner Jul 19 '13 at 16:04
  • @Neal in this example caching $(this) is not actually going to impact performance (much if any). – rlemon Jul 19 '13 at 16:04
  • @Neal I not trying to prove my point but don't tell me that we are now worried about nanosecond performance hits. If it was so important no one should actually use jQuery. – Learner Jul 19 '13 at 16:07
  • You don't even need the each anyway in this case. Just alert the index of the button. `alert( $(this).index(".button") );` – Kevin B Jul 19 '13 at 16:10
1

Welcome to asynchronous programming and global variables!

The problem you are seeing here is because of the fact that i is declared globally in this case, in that is accessible from anywhere by anything.

So, what happens when your script is executed is:

  • Loop through the array of classnames
  • On each iteration, bind a click to the matching node, calling the anonymous function you provided

The problem here is that the function you provided is executed outside of your loop (e.g. when the click happens), so the value of i is whatever it was in the last iteration, in this case 2

BenLanc
  • 2,344
  • 1
  • 19
  • 24