Don’t Use ‘Else {}’ (2019) | Hacker News Reader

Don’t Use ‘Else {}’ (2019)

medium.com

11 pointsreefer17 comments

The author hasn't learned from past coding practice mistakes.

Having multiple exit points makes the code harder to comprehend and figure out what it's doing. A return in the middle of a method makes the code more obtuse, not easier to read. Even if each method is clean and does one thing, reading through multiple functions and figuring out all the return points and where things stop is a huge enabler of bugs.

Personally I also find the use of negative clauses

    if !canDrink(age)

As a bad practice. However, since the author is attempting to not use 'else' he has to resort to this. Basically all his alternative cases have to be a negative if, or the method name needs to be negative

   if cantDrink(age)

Both of these scenarios create harder to read code. Comparing the below code, 'a)' is simpler to read and understand.

   // a)
   if canDrink(age) {
     okGiveBeer(to: name)
   } else {
     underAgeAlert(to: name)
   }
   // vs
   // b)
   if !canDrink(age) {
     underAgeAlert(to: name)
     return
   }
   okGiveBeer(to: name)

If I'm really trying to capture rules. I'll write that as. Sometimes it's easier to figure out complex rules, if there's 1 rule for every action instead of nested if else clauses.

  if canDrink(age) {
   okGiveBeer()
   return;
  }

  if underAge(age) {
    underAgeAlert()
    return
  }

  alert('Should never be here...')

Sorry, this is bad code. In the original version it's clear that there's only two possible outcomes: either you can drink, or you can't. Your version uses two different conditions, which makes four possibilities, which makes the code harder to reason because every reader must stop and think "Wait, can someone be underage and still allowed to drink? Or vice versa?"

Eliminate edge cases, especially those that should not arise in practice.

Single entry, single exit is important. Your code snippet essentially have three (3) exits. The two returns and the alert, which we can assume it throws an Exception or error that halts the code.

This is problematic because you have added excess code that doesn't help readability and makes bugs harder to track if an issue does come up. If anyone wants to add more rules the code will get even worse. Instinctually you recognize this because you added 'alert' it's your way of saying, if I have a bug I want to do something about it. Why is that? It's because you recognize that both if clauses might be false. If the alert triggers is it because 'canDrink' has a bug or 'underAge'?

Let's say 'underAge(age)' returns true if age < 21, because you live in the United States. That also implies that 'canDrink(age), returns true if age >= 21. Hopefully you can see that both methods test the same thing. That usually makes my spider sense tingle, I am doing something programmatically wrong. However for the sake of this example let's say this got merged.

New feature comes in, the age of drinking in another country is 16. We need to support their localization. If you updated only 'canDrink(age)' method (age >= 16), everything will work as expected, the if would be true and give them beer. If you only updated 'underAge(age)' (age < 16) you would have problems, 'canDrink' would return false (16 is not greater than 21) and underAge would return false (16 is not less than 16), and you would hit your alert.

However if you were using a simple if/else clause

   if canDrink(age) {
      okGiveBeer()
  } else {
      underAgeAlert()
  }

There is one method to deal with, 'canDrink'. Which means for the example of need to localize to country X with drinking age of 16 would have a single method to update. That reduces the chance of bugs. If bugs do appear they are easier to locate (is my if evaluating true when it's not? Why?). As an added benefit you have less lines of code, there is no code jumps or incomplete flows and you never need an alert in the end just in case.

canDrink(age) ? okGiveBeer() : underAgeAlert()

In general you're right but I think for the specific pattern of checking preconditions and handling errors the author is right.

    if file_exists(path) {
       # use path ...
       db = get_conn()
       if db.is_connected() {
          # use db ...
       } else {
         ret = new Error("db")
       }
    } else {
       ret = new Error("path")
    }

    return ret

Every check we need to do increases our nesting and makes it look like it's got lots of different paths when really it has exactly one happy path and many error paths.

    if ! file_exists(path) [
      return new Error("path")
    }

    # use path ...

    db = get_conn()
    if ! db.is_connected() {
      return new Error("db")
    }

    # use db ...

This is exactly my rule -- only if/else if it's the happy path that's branching.

That's a fair rebuttal. Depending on the situation though 'if not this - throw error' can be mitigated. It's one of the reasons why I try to avoid nulls coming into the code, otherwise you have to null check everywhere.

At the risk of bike-shedding...

It's not exactly secret knowledge that having "if (!cond) return" at the start of a function can save you one level of indentation from the rest of the function. Sometimes the saving can be significant (4 spaces per line, for a hundred lines). Especially if you have multiple such conditions. Especially^2 if you are an old fart like me who prefers to have all their code fit inside 80-column width. (I prefer being able to examine at least three source files side-by-side, thank you very much.)

That said, it's a rather trivial observation, and just like everything else, should be used with moderation. So I'm not sure what's the big deal...

(OK, now get off my lawwwwwwwn...)

It's often hard for experienced people to understand what is trivial and non-trivial for less experienced people. This is why writing documentation is so hard when you're completely submerged into a project.

I am personally a fan of a single return statement. Abolishing intend and else branches may be nicer to read but it hides cyclomatic complexity. A complexity which will not vanish but is no longer visible.

I would like to postulate that having an else makes it easier to read. We think in if/else fuzzy scenarios, it's why it flows so well in code. If you were teaching someone about traffic lights you would verbally explain something like this:

   if (light == green) {
     go()
   } else {
     stop()
   }

Not this:

   if (light != green) {
     stop()
   }
   go()

...and we haven't handled what to do when the light is orange.

Edit: In total agreement about the the single return, which is why I ignored that part of your post.

Personally, I find it easier to parse if the conditional is not only exhaustive, but itself returns a value (functional languages like Ocaml, Scala, Elixir have this).

Judging by the number of long, goofy-looking nested ternary expressions I find in Javascript code, a lot of people feel this way.

In functional programming everything is essentially an expression. So coming from there to procedural programming will create patterns like that or ternary expressions.

I come from a pascal/assembler school of thinking which is favoured in the OO/procedural languages. So when a language is using a procedural sytax then I code also in that style.

Rubocop actually enforces this as a default style rule.

But note the idea only works in statement-oriented languages. When if ... else ... constructs are an expression, both arms need to be evaluated for type checking purposes, which means both arms need to exist. Then you have language like R where you're free to use if ... else ... as keywords and statement if you like, but it's generally advised to use the ifelse function instead because it auto-vectorizes over multi-dimensional arguments.

My personal rules of thumb are:

1. Keep the business logic as un-indented as possible.

2. If there are several branching happy-paths, give them the same indentation.

I generally choose the "early return" pattern if there is (1) more than one precondition that must be met before executing a (2) medium-sized "happy path" code body. In these situations, having the meat of your function inside a deeply nested flow control structure is unwieldy, and for indentation-sensitive languages it is far too easy to lose track of which condition body you're currently in. In Python, this has bitten me more times than I can count.

So, my standard "early return" pattern follows this prototype:

  if (!discriminatingPrecondition) {
    // Some cases we ignore intentionally.
    return false;
  }

  if (!criticalPrecondition) {
    std::err << "Error: Precondition not met!" << std::endl;
    return false;
  }

  if (!fixablePrecondition) {
    scheduleRemedyForPrecondition();
    return false;
  }

  // Happy path
  
  return true;

However, for code with a single precondition statement without specific remediation logic for each precondition, I might occasionally prefer:

  if (preConditionA && preConditionB && preConditionC) {
    // happy path
  } else {
    std::err << "Error: Preconditions not met!" << std::endl;
  }

In the above examples, single entry/exit forces a structure like this:

  if (preConditionA) {
    if (preConditionB) {
      if (preConditionC) {
        // happy path
        ...
        ...
        ...
      } else {
        std::err << "User not authenticated!";
      }
    } else {
      std::err << "No server connection!";
      scheduleServerConnection();
    }
  } else {
    std::err << "No network connection!";
    scheduleConnectionCheck();
  }

Now, outside of "precondition" checking, it makes sense to use if/else or switch statements when conditions lead to similar happy path bodies. For example:

  if (shouldStand) {
    stand();
  } else {
    // No implied difference of statement body importance based on indentation
    sit();
  }

Versus:

  if (shouldStand) {
    stand();
    return;
  }
  // Hmm, is this path significantly different from the one above?
  sit();

So, in the "branching happy path" situation I opt for the former.

It's a design pattern that has it's uses. I've written plenty of code which goes through many precondition checks and has to throw appropriate errors or perform in-line fixups in the happy path, and this is the canonical way such things should be structured.

But as soon as the branching paths start splitting up and having different side effects or return different non-bailing conditions; then yeah, I'd quickly abandon that pattern.

What's the simplest plugin I can use on Firefox that prevents me from seeing posts from chosen domains such as this on HN? It is clear that the risk of my losing out on a good post from domains such as this is minimal. I got excellent results from blocking comments on YouTube, as there is statistically negative benefit in reading them, and I am now hoping for similar results on HN.

Reply on news.ycombinator.com