JSHint: Don't make functions within a loop

Question

What would be the correct way to solve the JSHint error in this case? Would removing function(i) solve it? And does having it this way hinder performance?

for (var i = 0; i + 1 <= pinlatlong.length; i++) {  
    (function(i) {
        setTimeout(function() {
            var latlong_array = pinlatlong[i].lat_long.split(','),
                    marker = new google.maps.Marker({
                    position: new google.maps.LatLng(latlong_array[0],latlong_array[1]),
                    map: map,
                    animation: google.maps.Animation.DROP,
                    icon: pinimage,
                    optimized: false
            });

            // info windows 
            var infowindow = new google.maps.InfoWindow({
                content: pinlatlong[i].title,
                maxWidth: 300
            });

            infoWindows.push(infowindow);

            google.maps.event.addListener(marker, 'click', (function(marker, i) {
                return function() {
                    infoWindows[i].open(map, this);
                };
            })(marker, i));

        }, i * 250); // end setTimeout
    }(i)); // end auto function
} // end for

[Search for messages](http://stackoverflow.com/search?q=%22Don%27t+make+functions+within+a+loop%22) *before* asking. — user2864740, Sep 01 '14 at 19:26
Personally I think that "hint" is often best ignored, but there are many duplicates explaining the cause and some solutions. If the outer (`function (i)`) function was removed then all the setTimeouts would use the same `i` because a new `i` would *not* be introduced. In this case it may also be prudent to use setInterval and then advancing `i`/the-point inside, calling clearInterval when done. — user2864740, Sep 01 '14 at 19:27
@user2864740 The hint is because the "outer function," the anonymous closure, is reevaluated and recreated with each iteration of the loop. The suggestion isn't to remove the function entirely. It's to define it outside the loop, so it's only created once, and have the loop just invoke it. — Jonathan Lonowski, Sep 01 '14 at 19:33
@JonathanLonowski But that "hint" is damn near misleading. Many people incorrectly assume (perhaps because of said "hints") that creating a new function-object is somehow expensive [in a way that will negatively effect performance] but *it is not*. Modern browsers deal with this very well so unless it's iterating the pixels in an image (for example) it Just Doesn't Matter. — user2864740, Sep 01 '14 at 19:35
@user2864740 Is it going to bring your system to a crawl recreating the `function`? No. But, it's still wasteful and for no real gain. Refactoring the code so the `function` isn't embedded in the loop can help with the readability, reducing the indent depths involved. — Jonathan Lonowski, Sep 01 '14 at 19:40
@JonathanLonowski It does **not** "recreate a function". That wording the source of perpetuating this "hint". There is *one* function-body in the internal implementation (of any implementation worth considering) that, when bound to different contexts, results in different function-objects. The cost is effectively *only* the closure context and extra function invocation - in the posted code the closure context is *important* for it to function as written. While this is "more than nothing", it often Just Doesn't Matter. — user2864740, Sep 01 '14 at 19:51
@JonathanLonowski Since the closure context *does* matter in this case (as written), then "defining [the function] outside the loop" would have *no* overall effect because a closure (albeit a smaller one) would have to be added in the loop - or values would have to be passed via another means. (In this case setTimeout does accept additional arguments to pass as parameters, but that is changing the code and not simply where said function is defined.) — user2864740, Sep 01 '14 at 19:52

score 1 · Accepted Answer · edited May 23 '17 at 12:09

If the outer (function (i)) function was removed then all the setTimeouts would use the same i because a new i (from the function parameter) would not be introduced (and it would lead to problems like this). As such the "hint" cannot be eliminated by simply removing it the outer anonymous function without changing other code as well. (Also see How do JavaScript closures work?)

While I disagree with this "hint" in general (and several other recommendations JSLint has), here are several different methods in which the "hint" can be avoided in this particular case besides merely disabling/ignoring the "hint".

One way to avoid the warning is to use setInterval once and using only a single callback function. The iteration (over i'th/the-point) is then advanced inside the callback inside and clearInterval is used when done. Note that the intent here is not to "improve performance" or eliminate the "hint" but show an alternative approach that some may perfer or find cleaner.

function addPins (map, pinLatLong, infoWindows) {
    // In separate function so these variables are guaranteed to be
    // in a new function scope.
    var i = 0;
    var timer = setTimeout(function() {
        if (i == pinLatLng.length) {
           clearTimeout(timer);
           timer = null;
           return;
        }

        var latLng = pinLatLong[i]; // only use `i` here

        var latlong_array = latlong.lat_long.split(','),
        // If combining `var` statements, which is another hint I disagree with,
        // consider NOT using a previously introduced var's value immediately as it
        // makes it harder to see the introduction (of latlong_array in this case).
            marker = new google.maps.Marker({
                position: new google.maps.LatLng(latlong_array[0],latlong_array[1]),
                map: map,
                animation: google.maps.Animation.DROP,
                icon: pinimage,
                optimized: false
            });

        // info windows 
        var infowindow = new google.maps.InfoWindow({
            content: latlong.title,
            maxWidth: 300
        });

        infoWindows.push(infowindow);

        // Eliminated use of extra uneeded closure as all the variables
        // used are contained within the callback's function context.
        google.maps.event.addListener(marker, 'click', return function() {
           infoWindow.open(map, this);
        });

        i++;
    }, 250);
}

As a bonus it avoids the "hint" which I personally disagree with in general cases. Go ahead, create hundreds of function-objects: JavaScript code does this all the time.

A second way is to use the support setTimeout has for additional arguments that it will supply as callback parameters. Thus the code could also be modified as so without the extra function (i) that was causing the "hint" warning. While this does create many timeouts (as in the original) it only uses a single function for the callback and avoids the extra closures.

function showPin (i) {
    // Everything that was in the setTimeout callback
    // ..
}

for (var i = 0; i + 1 <= pinlatlong.length; i++) {  
    setTimeout(showPin, 250, i);
}

Another way is to trick by creating the closure in another function, which effectively does the same thing as the original code. Arguably this is cleaner and easier to read than the original, even if not trying to eliminate the "hint" per-se.

function mkShowPin (i) {
    return function () {
        // Everything that was in the setTimeout callback
        // ..
    }
}

for (var i = 0; i + 1 <= pinlatlong.length; i++) {  
    setTimeout(mkShowPin(i), 250);
}

JSHint: Don't make functions within a loop

1 Answers1