S1126 decreases readability in methods using guards

Rules and Languages Report False-positive / False-negative...
1

Make sure to read this post before raising a thread here:

Then tell us:

  • What language is this for?
    JavaScript / TypeScript (ESLint)
  • Which rule?
    S1126. ‘sonarjs/prefer-single-boolean-return’
  • Why do you believe it’s a false-positive/false-negative?
    In certain cases this rule makes the code more difficult to read
  • Are you using
    “eslint-plugin-sonarjs”: “2.0.4”
  • How can we reproduce the problem? Give us a self-contained snippet of code (formatted text, no screenshots)
type Book = {
  status: "pending" | "created";
};
type User = {
  status: "pending" | "created";
  books: Book[];
  name: "Billy";
};

const isValid= (user: User) => {
  for (const book of user.books) {
    if (book.status === "pending") {
      return false;
    }
  }

  if (user.status === "pending") {
    return false;
  }

  if (user.name.includes("admin")) {
    return false;
  }

  return true;
};

Writing code with early returns / guards are common and readable, if you do it right. Wikipedia says it leads to flatter code with less nesting. In the example above there are checks, and only if those passes we know the data is valid.

The ESLint rule sonarjs/prefer-single-boolean-return wants to merge the last lines into the following:

return !(user.name.includes("admin"));

I think this makes the code harder to read.

  • It is more difficult to recognize that the isValid function is structured with early return guards
  • More difficult to read what scenario leads to true
3

Hi Stian,

I think this is a matter of taste, and hard to say if this is FP or not. I don’t see how we could change the rule to not report on such cases and still be relevant in other cases.

If you don’t like the rule, I would suggest disabling it.

5

saberduck

I don’t see how we could change the rule to not report on such cases and still be relevant in other cases.

Maybe the other cases are not relevant either, and the rule should not be included by default.

Have you read your own documentation about it? If not, let me show you something interesting.

First we have the “harder to maintain, as it adds unnecessary lines of code that need to be read and understood” code sample:

if (expression) {
  return true;
} else {
  return false;
}

Then the documentation recommend this code “if the caller expects a boolean and the result of the expression is not a boolean”:

return !!expression;

There is no world, no conceivable alternate reality, no heroic fantasy magical plan where this monstruosity is more readable than the so-called invalid sample.

Also, in the code sample shared by Stian, it would take more time to add a case with the solution provided by the rule. You can try it by yourself: add another condition to the mix that must be considered after the name and see how much code you have to change.

The code sample shared by Stian is more readable, and easier to maintain that the solution proposed by the rule.

2 Likes
6

saberduck

I had the impression that Sonar linter rules existed to catch bugs and vulnerable code. It seems I got that wrong, the README does mention clean code. As your were kind to point out, some of the rules are just random style preferences of people at Sonar company. That is ok.

The question that I have to ask myself is; why should I use rules from Sonar? Is Sonar known to be competent in frontend? Would it perhaps be wiser to find rules from a company that is known to be competent at frontend?

These are honest questions. I can of course disable the style rules that are pointless. That way I would still get rules for bugs and vulnerabilities.

1 Like