7

Possible Duplicate:
Assign click handlers in for loop

I need help with a loop in my code.

I loop through an array and add on clicks to divs. But it always adds onclicks to the last cycle through the loop and effectively cancels out the ones before it.

So i have this as my loop:

    start = 0;

for(i = start; i < (start+8); i++){ //8 per page
    if(i == array.length){ 
        break;  //page end
    } else {
        (function(i){
             document.getElementById('cell'+i).onclick = function(){ select(i); }
        })(i);  
    }
}

What occurs here is div id cell7 gets the on click added, but the div ids cell0 to cell6 do not. I'm guessing its something to do with the fact that i changes in the loop and so is also effected the i in the function ?

How do I fix this problem ?

Improve this question
20
  • I don't use jquery or any library for that matter so its not really useful to point to jquery's solution. Commented Oct 9, 2012 at 3:21
  • @Dave: Is this your actual code? you're closing over the i variable, so each iteration creates a separate scope with its own i separate from the outer loop's i. Commented Oct 9, 2012 at 3:22
  • Yes this is my code - im not following regarding the closing over the i? Commented Oct 9, 2012 at 3:23
  • @user1689607 hmm yes that's true. Commented Oct 9, 2012 at 3:26
  • @Dave The immediately invoked function creates a new variable scope with its own i variable. The handlers created inside that function invocation closes over the local i variable, and keeps its reference to it. Basically, you've shadowed the outer i with an inner i so your handlers will reference that one and work properly. Commented Oct 9, 2012 at 3:26

3 Answers 3

Reset to default
4

You're close:

(function(i){
    document.getElementById('cell' + i).onclick = function(){ select(i); }
})(i);

I think what you want is:

document.getElementById('cell'+i).onclick = (function(i){
    return function(){ select(i); }
})(i);

Also, because you have:

for(i = start; i < (start+8); i++)

i is not declared, so it becomes global. Declare 'i' to keep it local (presuming this is function code):

for (var i = start; i < (start+8); i++)

If it's global code it won't make any difference (but declare it anyway).

Edit

The above doesn't fix your issue, it just changes the syntax.

The following "works" for me, cell0 to cell7 get a listener, the rest don't. select shows the correct value (0 to 7 depending on which one is clicked on).

<script>
    function addListener() {
      var start = 0;
      for(i = start; i < (start+8); i++){ 
        document.getElementById('cell'+i).onclick = (function(i){
            return function(){ select(i); }
        })(i);  
      }
    }
    function select(arg) {
      alert(arg);
    }
    window.onload = addListener;
</script>

<p id="cell0">0</p>
<p id="cell1">1</p>
<p id="cell2">2</p>
<p id="cell3">3</p>
<p id="cell4">4</p>
<p id="cell5">5</p>
<p id="cell6">6</p>
<p id="cell7">7</p>
<p id="cell8">8</p>
<p id="cell9">9</p>
Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

Still same issue =/ It only assigns to the last div the rest don't do any thing.
When i simplified it to this : document.getElementById('cell'+i).onclick = select(i); It did as it should except the functions ran straight away (obviously), yet using the above method u suggest only makes it work on the last div in the loop.
There is something you're not showing, perhaps related to if(i == array.length). The example I posted "works".
2

Try this example. var i = 1 assuming your HTMLElement id starts at 1. Change to reflect the situation.

var select = function(i) {
    console.log(i);
};

var arr = ["1", "2", 3, 4, 5, 6];

var start = 0;
var max = 8;

for (var i=1; i<(start + max); i++) {
    if (i === arr.length) {
        break;
    } else {
        (function(i) {
            var element = document.getElementById(['cell', i].join(''));
            element.onclick = function() {
                 select(i);  
            };
        })(i);
    }
}

​ Calling an external function should be a faster implementation, however.

Improve this answer

2 Comments

Seems to work for me. Just curious: why use ['cell', i].join('') instead of 'cell' + i? I've seen it more and more lately.
I do quite a bit of node.js and during some research, I stumbled on this Google writeup. I've been using it ever since and it's now a convention of mine.
1

I'm not sure why but these suggestions didn't fix my problem - but i managed to find a way around it by adding to the onclicks as i display the divs during the loop so i ended up doing:

innerHTML = '<Div onclick="">'; //etc etc

Instead of adding the divs to the output "then" assigning the onclicks later... don't know if my new method is the best choice but it works so i'll have to stick to it!

Improve this answer

2 Comments

Yours and everyone else's demos worked for me. Are you using IE6 or something? Btw, tag names should always be lowercase, ie: <div onclick="">
Thanks for the tip with the lower case @MiniGod

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.