Hacker News new | ask | show | jobs
by brownbat 2080 days ago
Another design philosophy is that there should be one and only one exit point to a function whenever possible. Maybe that's fallen out of favor, or not important to most folks. But even if it's just a "nice-to-have," it's still a tradeoff here.

If every if block includes a return statement, you're basically writing a case switch or a series of elifs anyway. The readability of those structures is highly dependent on the language and how it expects serial conditionals to be most efficiently written.

3 comments

To me the right answer is the one that works and is most readable.

Personally, I find there are other factors that are far more important than early vs single return, such as nesting, doing too many things, unit tests, matching style of the codebase, and just being too long. If your method fits on a screen and only has one or two levels of nesting, it's usually easy to comprehend no matter what. If anything, I usually find "one return" to make things longer, as it requires additional variable declarations, and sometimes is downright awkard (eg: returning from a switch statement).

You can rewrite the entire example using a single return with conditional (ternary) operators, but how does that affect the readability? It probably depends more on if that's common in the codebase, and how the developer uses spaces, brackets and newlines then anything else.

Unit tests also go a long way here. They prove each branch of the logic works and ensure it stays working if someone decides to change the style in the future. They also force the code to be written in a way that's testable which (generally) means it'll naturally be narrow in scope, loosely coupled and short. I'll take ugly but unit tested over "meets project style guide" code any day of the week.

>The readability of those structures is highly dependent on the language and how it expects serial conditionals to be most efficiently written.

My knowledge is limited only to c# thus c style languages so i am curious how you would write the contrived conditional statements in the article to be more readable in any other language that would make it more readable than guard clauses

In Haskell (I know, I know) I would probably write something like this, if I were porting the example one-to-one:

  getInsuranceAmount status = case
      [hasInsurance,  isTotaled,  isDented,   isBigDent   ] <*> [status] of
      [False,         _,          _,          _           ] ->        1
      [True,          True,       _,          _           ] ->    10000
      [True,          False,      False,      _           ] ->        0
      [True,          False,      True,       True        ] ->      270
      [True,          False,      True,       False       ] ->      160
Personally, I'm not a fan of the early return pattern, since it forces me to read the entire function top to bottom (and possibly simulate a state machine in my head) to understand under what circumstances it returns each possible value, and I'm far too lazy for that. In contrast, my example makes it very straightforward to see what set of conditions leads to which value, and vice versa.

Tangentially, this probably wouldn't be a very idiomatic piece of code to write in Haskell, since some of the conditions are likely to be mutually exclusive (what does it mean when isDented is False but isBigDent is True?) My instinct would be to model the status as a sum type, and then to pattern match on its inhabitants instead of writing helpers for 'hasInsurance,' 'isTotaled,' etc, but someone with proper professional Haskell experience might have an even slicker way to think of it.

I don't necessarily think what you don't like is related to early returns because the code in the article could be written like

  if (!status.hasInsurance()){
    return 1;
  }
  if (status.hasInsurance() && status.isTotaled()){
    return 10000;
  }
  // ...
to be explicit about the conditions. Or without early returns but with still the same issue:

  if (!status.hasInsurance()) {
    amount = 1;
  } else if (status.isTotaled()) {
    amount = 10000;
  }
  // ... return amount
I think both have their place.

I like using guard clause style for functions which are mostly pure/functional, and one exit at the bottom style for functions that are mostly impure/procedural.