1

my question may sound like a question about closure but it does have some difference.

var people = [{name: 'john', age: 20}, 
    {name: 'james', age: 25}, 
    {name: 'ryan', age: 19}];
var mainDiv = document.createElement('div', {id: 'mainDiv'});
for (var i = 0; i < people.length; i++) {
    var button = document.createElement('button', {}, mainDiv);
    var person = people[i];
    button.onClick = function (e) {
        console.log('printing name');
        console.log(person.name);
    };
}

This is just an example I made up. Since the person variable is pointing at the last object of the people array, at the end of the for loop, it always prints out the last element when the button is clicked. However what I wanted to achieve is to print out each corresponding person's name when each button is clicked. What should I do to make the inner 'onClick' function to point at the right object?

Eugene Yu
  • 3,708
  • 4
  • 21
  • 27
  • I found my code is written wrong. here's the corrected jsfiddle version. http://jsfiddle.net/WYg92/ – Eugene Yu Jan 20 '14 at 00:56
  • I'm not sure where you get the arguments in `createElement`, is it a custom version? AFAIK it only takes one argument, a tagName. The ids don't show up in the fiddle. – elclanrs Jan 20 '14 at 00:57
  • @elclanrs oh yes you are right, I confused it with dojo's domConstructor which takes three arguments at once to minimize the lines of your code. – Eugene Yu Jan 20 '14 at 00:59
  • You could make the problem more obvious for yourself by moving `var person` to the top of the function, as that is where the variable is effectively declared. Even though you're declaring `var person` *inside* the loop, there is only one variable `person` shared by all iterations of the loop. All of your click handlers close over this *same* variable, which is why they all wind up with the last value. This would be considered a best practice by many seasoned JavaScript developers. – user229044 Jan 20 '14 at 01:02
  • It seems to be a dup question. I may close the question. and Thanks to all the answers the answers are all well written! – Eugene Yu Jan 20 '14 at 01:13

3 Answers3

1
for (var i = 0; i < people.length; i++) {
    var button = document.createElement('button', {}, mainDiv);
    var person = people[i];

    var clickfunc=function(name){return function(e){
        console.log('printing name');
        console.log(name);
    }}

    button.onclick = clickfunc(person.name);
}

You're right this is not a typical closure function, but a common misuse of closure where the variables are supposed to be pre-bound.

When calling the clickfunc function, person.name is passed to the call stack with the value AT THE TIME OF CALLING. The function itself returns another function, so that you're not invoking it until the button's actually clicked.

I have also corrected onClick to onclick.

Schien
  • 3,855
  • 1
  • 16
  • 29
1

You can break the closure to the outer person using an immediately invoked function expression, e.g.:

button.onclick = (function (p) {
      return function (e) {
        console.log('printing name');
        console.log(p.name);
      };
}(person));

Incidentally, javascript is case sensitive so you want to assign to the onclick property, not onClick.

You also seem to be using or assuming extensions to the DOM specification, e.g.

document.createElement('div', {id: 'mainDiv'});

that is not standard javascript and seems to assume extensions to host methods.

RobG
  • 142,382
  • 31
  • 172
  • 209
0

The closure in the code needs to capture the person object within a new context. This can be done by creating a new function (outer) which returns a function (inner) and passing the arguments to that outer function. Also it is best to attach events using attachEvent or addEventListener instead of onclick. Additional calls to maindDiv.onclick will overwrite any eventhandlers on the element.

var people = [{name: 'john', age: 20}, 
        {name: 'james', age: 25}, 
        {name: 'ryan', age: 19}];
var mainDiv = document.createElement('div', {id: 'mainDiv'});
for (var i = 0; i < people.length; i++) {
    var button = document.createElement('button', {}, mainDiv);
    var person = people[i];
    attachEvent(button, "click",bindClickEvent(person));
}

//Outer function
function bindClickEvent(person){
   //inner function retains context of outer function
   return function(){
        console.log('printing name');
        console.log(person.name);
   };
}

function attachEvent(elem, event, func){
    if(elem.attachEvent){
      elem.attachEvent("on" + event, func);
    }else{
        elem.addEventListener(event, func);
    }
}

JS Fiddle: http://jsfiddle.net/vD5B7/1/

Kevin Bowersox
  • 93,289
  • 19
  • 159
  • 189
  • 1
    Nitpick: The event handler in the OP's code already is a closure. The solution is to create a new context where the value of the variable doesn't change. – Felix Kling Jan 20 '14 at 01:51
  • Closure allows visibility of outer variables. by definition it doesn't bind values. Felix is right, by creating a new context, or in my words, by keeping the values on the call stack, the variable is bound at definition time, not call time. Also changing onclick is fine and concise. It's a matter of perspective and mental model. Event firing is DOM, onclick is also an attribute in a JavaScript object. Functions are first-class in JavaScript and can be assigned to event handlers easily. – Schien Jan 20 '14 at 04:14
  • @Schien The reason I suggested not using onclick is that if an additional `onclick` event handler is added to the element it will overwrite the existing event handler – Kevin Bowersox Jan 20 '14 at 11:56
  • @FelixKling Thanks for the correction. There seems to be a lot of terminology out there that people use to describe closures. – Kevin Bowersox Jan 20 '14 at 12:01