Passed the function in AddEventListener

Question

When I passed my function in addEventListener() method, it don't work right. Event don't register, and my function don't call.

code

 <div id="box-wrap">
        <ul id="colorize">
        </ul>
</div>

JavaScript

function colorize(){
var ul = document.getElementById('colorize');

for(var i = 0; i < 36; i++){
    ul.appendChild(document.createElement('li'));
}

function randomColor(li){
    li.style.background= "#"+(Math.random()*0xFFFFFF<<0).toString(16);


}
var liElements = ul.children;
for (i = 0; i < liElements.length; i++){
 liElements[i].addEventListener('mouseover',randomColor(liElements[i]),false);
    }

}

What is wrong?

possible duplicate of [Javascript closure inside loops - simple practical example](http://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example) — Benjamin Gruenbaum, Aug 06 '13 at 08:55
`addEventListener('mouseover',randomColor(liElements[i]),false)` **calls** `randomColor` and passes its return value into `addEventListener`, exactly the way `foo(bar())` **calls** `bar` and passes its return value into `foo`. — T.J. Crowder, Aug 06 '13 at 08:55
Your random colour generator is a tad off: it may yield something like `#34b9d`, perhaps use `#"+('000000'+((1<<24)*Math.random()|0).toString(16)).slice(-6)`or, sticking with your code: `('000' + (Math.random()*0xFFFFFF<<0).toString(16)).slice(-6)` around your code — Elias Van Ootegem, Aug 06 '13 at 10:28

Halcyon · Answer 1 · 2013-08-06T09:04:24.887

2

The 2nd argument to addEventListener must be a function, you're giving it undefined (the output of randomColor(..)

Call it like this:

liElements[i].addEventListener('mouseover', function () {
    randomColor(liElements[i]);
} ,false);

And now you'll run into a closure problem (i has the wrong value), fix like so:

(function (bound_i) {
    liElements[bound_i].addEventListener('mouseover', function () {
        randomColor(liElements[bound_i]);
    } ,false);
} (i)); // <-- immediate invocation (IIFE)

edited Aug 06 '13 at 09:04

answered Aug 06 '13 at 08:54

Halcyon

57,230
10
89
128

1

Or even better, with a forEach which already introduces a closure :) – Benjamin Gruenbaum Aug 06 '13 at 08:55
And if not using `forEach`, at least make the factory function something named and outside the loop. There's no need to create a new *factory* function on every loop iteration, just a new event handler function. It's also best practice to use a **different** name for the argument, for clarity. – T.J. Crowder Aug 06 '13 at 08:56
Or better still: _delegate_ the event, instead of binding all those individual listeners... – Elias Van Ootegem Aug 06 '13 at 08:57
@TJCrowder I agree that it improves performance (marginally) but it doesn't improve readability. You're welcome to write your own answer. @Elias you need to bind `i` somehow, how would you do it? – Halcyon Aug 06 '13 at 08:57
Here is [**what I'd do**](http://jsfiddle.net/HCK8b/) in a similar solution to this. – Benjamin Gruenbaum Aug 06 '13 at 09:00
@FritsvanCampen: Check my answer and linked fiddle, there's no need for a closure to get around the loop-issue: there's only 1 event listener. If the target of the mouseover is a `li`, then change its colour, it doesn't matter which `li` it is – Elias Van Ootegem Aug 06 '13 at 09:05

score 0 · Accepted Answer · answered Aug 06 '13 at 08:59

Try instead of

liElements[i].addEventListener('mouseover',randomColor(liElements[i]),false);

using bind (be ware that this does only work in modern browsers), but the link has a fall back implementation for it)

liElements[i].addEventListener('mouseover',randomColor.bind(this, liElements[i]),false);

Passed the function in AddEventListener

2 Answers2