Why shouldn't I make functions in a loop?

Question

Jshint.com suggests this for the code below:

Line 150: }, false );

Don't make functions within a loop.

But, it allows me to not have to write document.getElementById() multiple times, instead I can save the ids in an array and loop through them.

More concise and maintainable code I feel.

function styleTwitter( pair_array )
{
    var i, input, label;
    for ( i = 0; i < pair_array.length; i+=2 ) 
    {
        input = document.getElementById( pair_array[ i ] );
        label = document.getElementById( pair_array[ i + 1 ] );

        input.addEventListener( "focus", function()
        { 
            if( input.value === '' )
            {
                label.style.opacity = 0; 
                input.style.border = '1px solid #888888'; 
            }
        }, false );

        input.addEventListener( "blur", function()
        {
            if( input.value === '' )
            {
                label.style.opacity = 1;
                new EffectsFont( label ).fade( 'up', 150 );
                input.style.border = '1px solid #dddddd'; 
            }

        }, false );
    }

just updated it...i'm writing it..have not ran it yet..point is...instead of calling document.getElementById multiple times...I just want to loop through the ids and set in a loop. — CS_2013, May 19 '12 at 20:26
Duplicate to http://stackoverflow.com/questions/6865798/dont-make-functions-within-a-loop-jslint-error http://stackoverflow.com/questions/7103720/question-on-jslint-error-dont-make-functions-within-a-loop http://stackoverflow.com/questions/3037598/how-to-get-around-the-jslint-error-dont-make-functions-within-a-loop — check out the answers — alf, May 19 '12 at 20:27
Try the code and then see why JSLint complains. I'm going to bet that the code doesn't do what you expect it should do. — Blender, May 19 '12 at 20:28
well ..i just don't want to have to call document.getElementById 4-9 times for each Model in my MVC. — CS_2013, May 19 '12 at 20:29
Don't apply the event handler in the loop. Make one outside of the loop. With jQuery, it would be something like `$('input').blur(function() {...`. — Blender, May 19 '12 at 20:30
this is just an initialization of style for Model/Forms on my page. I wanted a simple way to apply the same style to all the inputs for each Model/Form which calls it. My way is simple and increases readability and maintainability, with no downside. — CS_2013, May 19 '12 at 20:35
It's not the event handler, it is the function definition that I need to take out. — CS_2013, May 19 '12 at 20:38
I don't know about javascript but in other languages event handler is called with something like sender or e.Source in arguments so you can use the same handler (function) for every object, you just check value of sender or e.Source and apply your actions to it. — zduny, May 19 '12 at 20:48
@CS_2013 , Can you use jQuery ? I do not know a cross browser way in pure javascript — Jashwant, May 19 '12 at 20:59
I have one working case and one broken case: http://stackoverflow.com/questions/10669340/calling-event-listeners-one-of-two-ways-works-whats-the-differnce — CS_2013, May 19 '12 at 22:40

alf · Accepted Answer · 2012-05-22T16:36:53.620

2

Have you tried creating one function which would analyse the event source and do the same? It will be both easier to grasp (read: even more maintainable), and it won't eat memory (the closure captures the whole stack, so it's never cheap).

edited May 22 '12 at 16:36

answered May 19 '12 at 20:26

alf

8,377
24
45

Nice thought, this should be the way +1. – Jashwant May 19 '12 at 20:59
...just pull out the possibly offending code to another function to isolate...good idea. – CS_2013 May 23 '12 at 14:09

score 0 · Answer 2 · answered May 19 '12 at 20:29

When you do DOM operations like getElementById() lookups, they are expensive and slow down the page. When you programmatically apply style changes like opacity in a loop, these too are expensive because the browser must repaint the screen each time.

Now, if you had a CSS class that applied these changes, you could apply all the changes in a single operation and save hundreds of browser repaint operations. That is principally why one should eschew looped UI updates.

However, it depends how many loops you're doing. If it does not slow things down and this is your only loop, don't worry about it. It is more of a concern for higher performance applications.

P.S. Keep in mind that when adding event listeners to elements like this, that you also remove them if the element is removed from the DOM later, our you'll have a memory leak.

I don't think it repaints the screen each time. Are you sure, it doesn't just repaint the element? How would functions like JQuery fade() work or any animations for that point? — CS_2013, May 19 '12 at 20:32
The browser is a huge image (buffer). When you hover over a link, a small rectangle of that image (the link) is repainted, not the whole screen. Hence, on some IE versions you'll see curious activity when using fade() due to the many repaints. Each time you update a property, the browser schedules a repaint of the affected area. However, if you hand over the fade animation to the browser using CSS3 transitions, the browser can optimize this process. — Joseph Lust, May 19 '12 at 20:38

Why shouldn't I make functions in a loop?

2 Answers2