I'm generating an unordered list through javascript (using jQuery). Each listitem must receive its own event listener for the 'click'-event. However, I'm having trouble getting the right callback attached to the right item. A (stripped) code sample might clear things up a bit:

for(class_id in classes) {
    callback = function() { this.selectClass(class_id) };
    li_item = jQuery('<li></li>')
                .click(callback);
}

Actually, more is going on in this iteration, but I didn't think it was very relevant to the question. In any case, what's happening is that the callback function seems to be referenced rather than stored (& copied). End result? When a user clicks any of the list items, it will always execute the action for the last class_id in the classes array, as it uses the function stored in callback at that specific point.

I found dirty workarounds (such as parsing the href attribute in an enclosed a element), but I was wondering whether there is a way to achieve my goals in a 'clean' way. If my approach is horrifying, please say so, as long as you tell me why :-) Thanks!

I'm generating an unordered list through javascript (using jQuery). Each listitem must receive its own event listener for the 'click'-event. However, I'm having trouble getting the right callback attached to the right item. A (stripped) code sample might clear things up a bit:

for(class_id in classes) {
    callback = function() { this.selectClass(class_id) };
    li_item = jQuery('<li></li>')
                .click(callback);
}

Actually, more is going on in this iteration, but I didn't think it was very relevant to the question. In any case, what's happening is that the callback function seems to be referenced rather than stored (& copied). End result? When a user clicks any of the list items, it will always execute the action for the last class_id in the classes array, as it uses the function stored in callback at that specific point.

I found dirty workarounds (such as parsing the href attribute in an enclosed a element), but I was wondering whether there is a way to achieve my goals in a 'clean' way. If my approach is horrifying, please say so, as long as you tell me why :-) Thanks!

• javascript
• jquery
• events
• iteration

• As Peter has said you have e across a closure problem. For more information on this K Scott Allen has a decent post here: odetocode./Blogs/scott/archive/2007/07/09/11077.aspx – KnackeredCoder Commented Jul 9, 2009 at 15:04
• actually, it looks like a simple scoping problem. see answer below. – geowa4 Commented Jul 9, 2009 at 15:08

6 Answers 6

This is a classic "you need a closure" problem. Here's how it usually plays out.

1. Iterate over some values
2. Define/assign a function in that iteration that uses iterated variables
3. You learn that every function uses only values from the last iteration.
4. WTF?

Again, when you see this pattern, it should immediately make you think "closure"

Extending your example, here's how you'd put in a closure

for ( class_id in classes )
{
  callback = function( cid )
  {
    return function()
    {
      $(this).selectClass( cid );
    }
  }( class_id );
  li_item = jQuery('<li></li>').click(callback);
}

However, in this specific instance of jQuery, you shouldn't need a closure - but I have to ask about the nature of your variable classes - is that an object? Because you iterate over with a for-in loop, which suggest object. And for me it begs the question, why aren't you storing this in an array? Because if you were, your code could just be this.

jQuery('<li></li>').click(function()
{
  $(this).addClass( classes.join( ' ' ) );
});

Your code:

for(class_id in classes) {
    callback = function() { this.selectClass(class_id) };
    li_item = jQuery('<li></li>')
                        .click(callback);
}

This is mostly ok, just one problem. The variable callback is global; so every time you loop, you are overwriting it. Put the var keyword in front of it to scope it locally and you should be fine.

EDIT for ments: It might not be global as you say, but it's outside the scope of the for-loop. So the variable is the same reference each time round the loop. Putting var in the loop scopes it to the loop, making a new reference each time.

This is a better cleaner way of doing what you want.

Add the class_id info onto the element using .data().

Then use .live() to add a click handler to all the new elements, this avoids having x * click functions.

for(class_id in classes) {
    li_item = jQuery('<li></li>').data('class_id', class_id).addClass('someClass');
}

//setup click handler on new li's
$('li.someClass').live('click', myFunction )

function myFunction(){
   //get class_id
   var classId = $(this).data('class_id');
   //do something
}

My javascript fu is pretty weak but as I understand it closures reference local variables on the stack (and that stack frame is passed around with the function, again, very sketchy). Your example indeed doesn't work because each function keeps a reference to the same variable. Try instead creating a different function that creates the closure i.e.:

function createClosure(class_id) {
  callback = function() { this.selectClass(class_id) };
  return callback;
}

and then:

for(class_id in classes) {
  callback = createClosure(class_id);
  li_item = jQuery('<li></li>').click(callback);
}

It's a bit of a kludge of course, there's probably better ways.

why can't you generate them all and then call something like

$(".li_class").click(function(){ this.whatever() };

EDIT:

If you need to add more classes, just create a string in your loop with all the class names and use that as your selector.

$(".li_class1, .li_class2, etc").click(function(){ this.whatever() };

Or you can attach the class_id to the .data() of those list items.

$("<li />").data("class_id", class_id).click(function(){
    alert("This item has class_id "+$(this).data("class_id"));
});

Be careful, though: You're creating the callback function anew for every $("<li />") call. I'm not sure about JavaScript implementation details, but this might be memory expensive. Instead, you could do

function listItemCallback(){
     alert("This item has class_id "+$(this).data("class_id"));
}

$("<li />").data("class_id", class_id).click(listItemCallback);