2

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?

Improve this question
5
  • I found my code is written wrong. here's the corrected jsfiddle version. jsfiddle.net/WYg92 Commented Jan 20, 2014 at 0: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. Commented Jan 20, 2014 at 0: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. Commented Jan 20, 2014 at 0: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. Commented Jan 20, 2014 at 1: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! Commented Jan 20, 2014 at 1:13

3 Answers 3

Reset to default
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.

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

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.

Improve this answer

1 Comment

Yes I was confused creating element with dojo. Thanks for pointing that:)
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/

Improve this answer

4 Comments

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.
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 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
@FelixKling Thanks for the correction. There seems to be a lot of terminology out there that people use to describe closures.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.