dart-lang / linter

Linter for Dart.
https://dart.dev/tools/linter-rules
BSD 3-Clause "New" or "Revised" License
627 stars 172 forks source link

proposal: `no_else_after_return` #4363

Open goderbauer opened 1 year ago

goderbauer commented 1 year ago

no_else_after_return

Description

Move the code of the else branch below the if block.

Details

When the if case always ends with a return, the else branch is not necessary and the code can instead be placed below the if block. This decreases code complexity and indentation and overall improves readability.

Kind

style advice, maybe?

Bad Examples

if (foo) {
  // lots of other code...
  return 'foo';
} else {
  return 'not foo';
}

Good Examples

if (foo) {
  // lots of other code...
  return 'foo';
}
return 'not foo';

Discussion

Real world example: https://github.com/flutter/flutter/pull/126935#discussion_r1195510913

Discussion checklist

srawlins commented 1 year ago

I think typically I would advocate that the former (bad) example is more readable in many cases.

In cases where a return is an early return that allows 100 lines following to be not indented 2 spaces, it makes sense to not use else. But often I like to use else just to improve readability, like if the then-block is long, or if the condition is long, it ties the state of the following code back to the condition.

Also, as this is a style suggestion, I think we need such a rule to be backed by a style guide, like Effective Dart.

bwilkerson commented 1 year ago

@munificent

incendial commented 1 year ago

jfyi, if you decide not to add this rule to the linter, it's already available in DCM https://dcm.dev/docs/rules/common/avoid-redundant-else/

eernstg commented 1 year ago

Drive-by comment: It sounds like this lint could be quite useful. It also reminded me of the idea that some language constructs could announce that "if we reach this point, we're done". Cf. https://github.com/dart-lang/language/issues/3080.

The point is that it is then very easy to see at a glance that a given if-statement is guaranteed to end the execution in the current function body, and this again provides a guarantee (at a glance) that the code that comes after the if-statement will be executed if and only if we would have executed an else part.

mateusfccp commented 1 year ago

I'm with @srawlins. I prefer the "bad" example most of the times. Obviously, there are cases where the readability is worse, specially as we increase the nesting of the condition. However, most cases I see are just like the example:

String something(bool condition) {
  if (condition) return 'Something';

  return 'Not something';
}

Using if-else in this case is way more readable IMO, as it makes it very clear that we have two possible cases. ifs with no elses are often used in cases where we have non-returning side-effects, and this is this the first thing that comes to my mind when I see an elseless if.

munificent commented 1 year ago

I think about this every time I have conditional logic that returns on both branches. I tend to lean towards the early return style suggested here, but I'm never fully convinced that it is actually better. I do appreciate the symmetry of having the returns in both branches at the same level.

I think this would be a reasonable lint to support but only if users have to explicitly opt in to enable it if they prefer that style and want consistency. I don't think we'd want "Effective Dart" to have an opinion one way or the other. I just don't think it matters enough either way to be worth bothering users.