S7728 FP Use "for...of" loops instead of "forEach" method calls

Rules and Languages Report False-positive / False-negative...
,
1
  • What language is this for? Javascript
  • Which rule? S7728
  • Why do you believe it’s a false-positive/false-negative?
    • This is a false positive because list.forEach((element, index, array) ... cannot always be replaced by for (const element of list) ...; the latter does not supply index or array.
  • Product: SonarQube Cloud
  • How can we reproduce the problem?
const list = ['a', 'b', 'c'];

list.forEach((element, index) => {
  console.log(`${element} at ${index}`)
}); 
// logs "a at 0", "b at 1", "c at 2"

for (const element of list) {
  console.log(`${element} of ${index}`);
}
// throws Uncaught ReferenceError: index is not defined
1 Like
3

Hello @zburke,

when using for..of, you would need to call entries() in order to get the index:

for (const [index, element] of list.entries()) {
  console.log(`${element} of ${index}`);
}
5

Please consider updating the documentation for this rule to include a list.entries() example. It’s an obvious solution in retrospect, but (for this noob at least) an easy one to overlook.

I still chafe at this rule because the situations Sonar identifies are all hypothetical, and in fact, none apply:

  • On a small list, function-creation overhead is not a legit issue. The very first rule of performance tuning is to measure first and make sure that you are applying optimizations where they will have the greatest effect. This is not that place.
  • In this loop, there is no control flow.
  • We aren’t using typescript.
  • The notion that code should be changed to make it “more readable and familiar to developers coming from other programming languages” is very close to saying “avoid using your language’s idioms if they’re not common in other languages” which is terrible advice.

While defensive coding is important, trying to handle every hypothetical seems foolish. We don’t need to treat every list like it might contain a million items, or every codebase as though it might, someday, be translated to another language. The original code was perfectly functional and known to be correct because (a) it came from production (in the PR in question, the offending block moved but did not change) and (b) our tests provide confidence that this (admittedly trivial) change will be fine.

Every change to a codebase comes with cost and risk. Here, it doesn’t feel like juice is worth the squeeze.

3 Likes
6

Hi @zburke,

Good point, I created a PR to add this.

Thank you very much for this feedback. We added many rules in the last couple of releases and this is the one rule that we considered the most opinionated and could raise some complains.

We still consider it be be overall a better practice than forEach, so we went for it. However we lowered the impact to “Low” because we agree that if someone is using forEach, most of the times this does not imply bad code.

Edit: Another point we did discuss and did not end up in the rule description was about async code. Of course not all forEach functions contain async functions, but it’s always possible to miss when you refactor and add some Promise. I will add this to the rule description.

8

I agree with this for existing working code, but not for new code. Although the effect is small on each instance, for loops are easier on the runtime than array methods taking functions, and over a large codebase there can be a measurable effect. Profiling will typically not help to apply this improvement after the fact since it’s spread over the code.

10

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.