JsHint (W083): Don't make functions within a loop. - Using [].forEach();

Question

I get this error:

JsHint (W083): Don't make functions within a loop.

when using the following code:

for (var prop in cmd.properties) {
  Object.keys(meta[prop].data).forEach(function (rule) {
    rules.data[rule] = meta[prop].data[rule] ? true : false;
  }.bind(this));
}

Basically I am looping through properties of an object meta[prop].data and for each property, I am setting true/false other another object properties using a ternary operator.

Reading some documentation I see:

JSHint and ESLint encounter a function expression in a for, while or do statement body.

• Is this error legitimate one?
• If yes, how to better re-write these lines?
• If no, how to disable this specific error validation using JsHint?

Answer 1

Is this error legitimate one?

Yes, you're declaring a function within a loop. On top of that, bind can be pretty expensive, since it has to create a new lexical scope every time and return a "new" function attached to that scope.

If yes, how to better re-write these lines?

If you can, declare the function before the loop and either bind once or use simple closure to avoid an explicit bind call:

var scope = this;
var ruleFunc = function (rule) {
  rules.data[rule] = meta[prop].data[rule] ? true : false;
}

for (var prop in cmd.properties) {
  Object.keys(meta[prop].data).forEach(ruleFunc);
}

I don't see where you're using this within the function, though, so you may be able to remove it entirely:

var ruleFunc = function (rule) {
  rules.data[rule] = meta[prop].data[rule] ? true : false;
}

for (var prop in cmd.properties) {
  Object.keys(meta[prop].data).forEach(ruleFunc);
}

Both of these will require refactoring your code, since you're using closure to grab the prop variable from the loop. You can use bind to work around that, with a performance hit:

var ruleFunc = function (prop, rule) {
  rules.data[rule] = meta[prop].data[rule] ? true : false;
}

for (var prop in cmd.properties) {
  Object.keys(meta[prop].data).forEach(ruleFunc.bind(this, prop));
}

You're also using a conditional to return true/false, which is a common smell. You typically want to convert that to a boolean, with !! being the idiomatic JS way:

rules.data[rule] = !!(meta[prop].data[rule]);

If you can, avoiding for ... in loops will usually make your life better, so you may want to refactor that as well:

Object.keys(cmd.properties).forEach(function (prop) {
  Object.keys(meta[prop].data).forEach(function (rule) {
    rules.data[rule] = !!(meta[prop].data);
  });
});

You may be able to improve that further still.