JsHint W083 Don't Make Functions in Loop and jQuery's $.each() function

Question

I'm currently using JsHint and am receiving warning W083: "Don't make functions within a loop". I read this post from JsLint Error Explanations and understand why you should not do this, which essentially boils down to the asychrnonous nature of JavaScript and the potential for variables to be overwritten.

However, I also read in a few other posts here on SO that although this is a faux pas it does not always lead to bugs depending on the situation.

My situation in particular that JsHint is complaining about is a for-loop that uses the jQuery $(selector).each() function within it. This function takes a function as a parameter. Below is a snippet of the code that I'm concerned about. Don't worry about what it actually does⁺ since I'm really just using this as an example:

for (var i = 0; i < sections.length; i++) {
    disableSectionState[sections[i].name] = {};

    $('.' + sections[i].name).each(function (index, element) {
        var id = $(element).attr('id');
        disableSectionState[sections[i].name][id] = $(element).attr('disabled');
    });

    if (sections[i].isChecked) {
        $('.' + sections[i].name).attr('disabled', 'disabled');
    }
}

Essentially, this is just a nested for-each loop within a for-loop, so I didn't think this would be too dangerous, but I'm obviously not familiar with all of the quirks in js. As of right now, everything is working properly with this function in particular, but I wanted to ask the community about the dangers of this using jQuery's each function within a loop.

To prevent this being marked as a dupe I did see this SO question, but the only answer doesn't go into any detail or explain anything, and based on the comments it looks like an XY problem anyway. I'm more interested in the why this is when at it's core is that it's essentially a nested loop.

Is it really that much safer for this function to be extracted and named outside of the loop? If I copied the loop counter to a variable in scope of the anonymous function, would that eliminate the potential danger of this design? Is that function executed completely asynchronously outside of the main for-loop?

⁺In case you're actually interested: This code is used to determine if certain fields should be disabled at page load if certain options are enabled.

Answer 1

JSHint is a very opinion-based syntax checker. It's kind of like deciding which type of citations to do on a paper MLA or APA. If you go with one, you just follow their rules because, most of the time, it is "right", but it's rarely ever wrong. JSHint also says to always use === but there may be cases to use == instead.

You can either follow the rules or ignore them with the following

// Code here will be linted with JSHint.
/* jshint ignore:start */
// Code here will be ignored by JSHint.
/* jshint ignore:end */

If you are going to use JSHint, I would just comply. It tends to keep the code a little more consistent, and when you start trying to work around one warning or error, it tends to start creating a bunch more

Is it really that much safer for this function to be extracted and named outside of the loop?

• In practice, yes. In general, on case by case, maybe not.

If I copied the loop counter to a variable in scope of the anonymous function, would that eliminate the potential danger of this design?

• No.

Is that function executed completely asynchronously outside of the main for-loop?

• Pretty sure it is.

Answer 2

The problem isn't using jQuery's each within the loop, it's repeatedly declaring a function. That can lead to some odd closure issues (the function closes on a reference to the loop counter, which still gets updated and can change) and can be a non-trivial performance problem on less clever VMs.

All JSHint is asking you to change is:

function doStuff(index, element) {
  var id = $(element).attr('id');
   disableSectionState[sections[i].name][id] = $(element).attr('disabled');
}

for (var i = 0; i < sections.length; i++) {
  disableSectionState[sections[i].name] = {};

  $('.' + sections[i].name).each(doStuff);

  if (sections[i].isChecked) {
    $('.' + sections[i].name).attr('disabled', 'disabled');
  }
}

Most of the dangers come when you're calling something asynchronously from within a loop and close over the loop counter. Take, for example:

for (var i = 0; i < urls.length; ++i) {
  $.ajax(urls[i], {success: function () {
    console.log(urls[i]);
  });
}

You may think it will log each URL as the requests succeed, but since i probably hit length before any requests have come back from the server, you're more likely to see the last URL repeatedly. It makes sense if you think about it, but can be a subtle bug if you aren't paying close attention to closure or have a more complex callback.

Not declaring functions within the loop forces you to explicitly bind or pass the loop counter, among other variables, and prevents this sort of thing from accidentally cropping up.

In some more naive implementations, the machine may also actually create a closure scope for the function every iteration of the loop, to avoid any potential oddities with variables that change within the loop. That can cause a lot of unnecessary scopes, which will have performance and memory implications.