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

Code Maverick
  • 20,171
  • 12
  • 62
  • 114
kevinius
  • 4,232
  • 7
  • 48
  • 79

5 Answers5

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.

Community
  • 1
  • 1
Pointy
  • 405,095
  • 59
  • 585
  • 614
  • Hi, thx for your answer... this fiddle shows what i was trying to achieve (click on the red -). http://jsfiddle.net/v4xz5/ – kevinius Dec 23 '13 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 '13 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.

Igor Raush
  • 15,080
  • 1
  • 34
  • 55
  • How would one make this less complicated, is there a more easier way to accomplish the same thing? – kevinius Dec 23 '13 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 '13 at 20:26
  • Yes, i have to update another html table with the same output but on another location. – kevinius Dec 23 '13 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 '13 at 20:50
  • Can't we simply say `x.addEventListener("click", function(e) { updateProduct(i); });`? I assume `i` comes from the outer loop. –  Dec 23 '13 at 21:09
  • No, this is the whole issue. Without producing a closure, the `i` inside the click handler refers to the loop index `i`, which will have changed by the time the handler is called. If you substitute your snippet into the fiddle, clicking the minus will alert `2` for both items (since that is the last value of the index). – Igor Raush Dec 23 '13 at 21:12
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

Mindastic
  • 4,023
  • 3
  • 19
  • 20
  • This will also not work. Read [this question](http://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example). – Pointy Dec 23 '13 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 '13 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);
    }
duckbrain
  • 1,219
  • 1
  • 13
  • 26
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);
    }
}
Beetroot-Beetroot
  • 18,022
  • 3
  • 37
  • 44