views:

175

answers:

4

Hi guys,

I am trying to dynamically change an element's onClick event and I have something like the following:

for (var i = 1; i < 5; i++)
{
    getElementById('element' + i).onclick = function() { existingFunction(i); return false; };
}

Everything seems to work fine apart from the fact that the argument passed to 'existingFunction()' is the final value of i=4 each time it is called. Is there a way to bind a function to onclick that uses the value of i at the time of binding as opposed to what it seems to be doing at the moment and referencing the original i in the for-loop.

Also is is there a way of performing the same bind without having to create anonymous functions each time? so that I can directly reference 'existingFunction' in each onclick for performance reasons?

Cheers guys, Yong

+3  A: 

Change

for (var i = 1; i < 5; i++)
{
    getElementById('element' + i).onclick = function() { existingFunction(i); return false; };
}

to

for (var i = 1; i < 5; i++)
{
    getElementById('element' + i).onclick = createOneHandler(i);
}

function createOneHandler(number){  
    return function() {  
        existingFunction(number); 
    }  
}

and it should work fine.

Working Demo

A good explanation is given here

JavaScript, time to grok closures

rahul
Haha, the page you referenced is almost same situation. I read through the article and it was also very helpful thanks for that.
yongjieli
+1  A: 

the code you posted should work the way you intended, your problem with i=4 is elsewhere. edit: this is wrong, rageZ is right about the scoping problem.

re the other question: all you can do is offload the verbosity with

var f = function (i) { return function () { existingFunction(i); return false; } }
for (...) { document.getElementById(...).onclick = f(i); }

BTW, you should use something like jQuery for DOM manipulation (concise syntax), and perhaps Zeta (http://codex.sigpipe.cz/zeta/) for the function composition

var f = compose(false_, existingFunction);
for (...) { $(...).click(f(i));
just somebody
+1  A: 

for the i being always 4, you have a scoping problem, I advise to read this. Scoping is are really important concept, so you have better to make sure to understand what's is going on.

a better code would be

for (var i = 1; i < 5; i++)
{
    getElementById('element' + i).onclick = existingFunction;
}

the onclick would pass an event has argument so you can know what element have been clicked i.e.

function existingFunction(event){
  // DO something here
}

you can read more about events there. IE does have the exact same event model as other browser so you would have to handle it.

Last bit, I advise you to use a JS framework(Jquery,ExtJS,DOJO,Prototype...) because it would simplify your task

RageZ
A: 

Hooray! It's loop closures again! See 422784, 643542, 1552941 et al for some more discussion.

is there a way of performing the same bind without having to create anonymous functions each time?

Yes, in ECMAScript Fifth Edition you get function.bind:

for (var i = 1; i < 5; i++)
    document.getElementById('element'+i).onclick= existingFunction.bind(window, i);

In the meantime since browsers don't yet generally support it you can monkey-patch an alternative implementation of bind (see the bottom of this comment for one such) built out of anonymous functions as a fallback.

Alternatively, assign the same event handler function to every element and just have it look at this.id to see which element number it is.

bobince