1

I'm trying to grasp this bit of code, I know it's a closure but I'm not getting the result I think I should get.

This code returns me an [object MouseEvent], I can't understand why?

I'm adding a function call (updateProduct) to an .addEventListener using this code, and it returns an [object MouseEvent]

function addEventListenerToMinPlus(){
    var x, y
    for(var i = 0; i < productItemAll.length; i++){
        x = productItemAll[i].querySelector(".boxNumbers-min")
        x.addEventListener("click", function(i){return function(i){updateProduct(i)}}(i))
        console.log(x)
    }
}

function updateProduct(jow){
    alert(jow)
}

jsFiddle

2
  • 2
    For the love of all that's holy, put those semicolons back in there. Semicolon elision is the devil. Dec 23, 2013 at 19:51
  • Yes, i'm planning to bring them back in on a production environment.
    – kevinius
    Dec 23, 2013 at 20:08

5 Answers 5

2

The browser invokes the event handler with an event object as the first parameter. Your function is declared to take a single parameter ("i"), so when you display it, that's what it is.

I suspect that what you meant was for the "i" inside the event handler to refer to the "i" in the outer function (the loop index). That also won't work, because the various handlers the loop creates will all refer to the same shared variable "i". See this old SO question.

2
  • Hi, thx for your answer... this fiddle shows what i was trying to achieve (click on the red -). jsfiddle.net/v4xz5
    – kevinius
    Dec 23, 2013 at 19:57
  • @kevinius right - did you understand my answer? There are two problems with your code: you've created your event handler function with "i" as a formal parameter, so "i" in the function will refer to whatever is passed to the function when the event is triggered. The second problem is that you'll run into problems due to the nature of JavaScript variable scoping. Read the answers to that linked other question.
    – Pointy
    Dec 23, 2013 at 19:59
1

The line

x.addEventListener("click", function(i) { return function(i) { updateProduct(i); }(i) }

produces a closure of the inner function

function(i) { updateProduct(i); }

The outer i is in the scope of this inner function, but it is shadowed by its parameter. So, in effect, the inner i represents the first argument passed to the click handler (the MouseEvent). If you want it to retain the value of the index, you have to change its name. Something like this:

x.addEventListener("click",
    function(i) { return function(e) { updateProduct(i); }(i)
}

Now, in the inner function, e is the MouseEvent, and i is the outer index. I have updated the JSFiddle: http://jsfiddle.net/Cdedm/2/. Clicking the minus alerts 0 for the first item and 1 for the second, as expected.

6
  • How would one make this less complicated, is there a more easier way to accomplish the same thing?
    – kevinius
    Dec 23, 2013 at 20:18
  • If you are trying to have the index of the item available from within its click handler, you have to produce a closure of some sort. The other option is looking up the index from within the click handler, since the handler knows about the element which was just clicked (available in e.target). Do you have to know the index of the element in your updateProduct function?
    – Igor Raush
    Dec 23, 2013 at 20:26
  • Yes, i have to update another html table with the same output but on another location.
    – kevinius
    Dec 23, 2013 at 20:37
  • In that case, this is not a bad solution. You could also use HTML5 data- attributes to reference the corresponding element in the other table, and then extract them from the click handler target in order to lookup the element you need to update. However, this will most likely end up being slower (and possibly messier).
    – Igor Raush
    Dec 23, 2013 at 20:50
  • Can't we simply say x.addEventListener("click", function(e) { updateProduct(i); });? I assume i comes from the outer loop.
    – user3458
    Dec 23, 2013 at 21:09
0

that is because you are sending the element as parameter.

You should try doing this:

function addEventListenerToMinPlus(){

    var x, y

    for(var i = 0; i < productItemAll.length; i++){

        x = productItemAll[i].querySelector(".boxNumbers-min")
        x.addEventListener("click", function(){return updateProduct(i)})

        console.log(x)


    }

}

Hope this works,

Regards,

Marcelo

2
  • This will also not work. Read this question.
    – Pointy
    Dec 23, 2013 at 19:59
  • Yes, you are right. I forgot the fact that when the click was being triggered, the "i" was going to be the length of the array due to closure. I think my example should work by setting a new variable inside the function being called and pass it to updateProduct. For example x.addEventListener("click", function(){var x = i; return updateProduct(x)});
    – Mindastic
    Dec 23, 2013 at 20:39
0

I think you are trying to do something like this. Your i will change by the time the click happens so it needs to be set to another local variable, in this case through the parameter on the new function. The click event handler will pass the event object you are getting currently

 function addEventListenerToMinPlus() {
        var x, y;
        for(var i = 0; i < productItemAll.length; i++) {
            x = productItemAll[i].querySelector(".boxNumbers-min");
            x.addEventListener("click", function(i){return function(){updateProduct(i)}}(i));
        }
    }

    function updateProduct(jow) {
        alert(jow);
    }
3
  • How would one make this less complicated, is there a more easier way to accomplish the same thing?
    – kevinius
    Dec 23, 2013 at 20:06
  • Just remove the i x.addEventListener("click", function(i){return function(){updateProduct(i)}}(i)); I will update the answer.
    – duckbrain
    Dec 23, 2013 at 20:07
  • hi, sorry, the question was actually for @igorraush... :p
    – kevinius
    Dec 23, 2013 at 20:19
0

Unless you really really really know what you're doing, you can play about with closures all day and still not get it right with this sort of thing.

A more understandable appraoch by far, with more readable code for most people, is to use jQuery's .data() method to associate data (i in this case) with the element in question so it can be read back when the click event fires, for example :

function addEventListenerToMinPlus() {
    var x, y;
    for(var i = 0; i < productItemAll.length; i++) {
        x = productItemAll[i].querySelector(".boxNumbers-min");
        x.data('myIndex', i);//associate `i` with the element
        x.addEventListener("click", function(i) {
            var i = $(this).data('myIndex');//read `i` back from the element
            updateProduct(i);
        });
        console.log(x);
    }
}

For the record, a working closure would be as follows :

function addEventListenerToMinPlus() {
    var x;
    for(var i = 0; i < productItemAll.length; i++) {
        x = productItemAll[i].querySelector(".boxNumbers-min");
        x.addEventListener("click", function(i) {//<<<< this is the i you want
            return function() {//<<<< NOTE: no formal variable i here. Include one and you're stuffed!
                updateProduct(i);
            }
        }(i));
        console.log(x);
    }
}

Not the answer you're looking for? Browse other questions tagged or ask your own question.