Else Considered Smelly | Hacker News Reader

Else Considered Smelly

wiki.c2.com

6 pointstodsacerdoti14 comments

I really dislike the term "code smell". Nothing wrong with the concept, but it tends to be used to make half-baked, patronizing remarks about other people's code. And when you try to argue there's nothing wrong with a particular piece of code, you get hit back with "it's bad and you just don't know it, and I can't explain it but it just is".

If you can't verbalize your reasons, it doesn't belong in a code review -- otherwise it's preference. And that's where people get confused about what a "code smell" is really supposed to be.

The article does not suggest an alternative. If I consider any typical real-life case where I would use else, trying to finagle the code into not needing else would make it smell considerably more.

Else is a common name in my part of the world and capitalizing the word makes the connection more explicit.

i agree about the "first smell", the inverted condition, and fairly recently started doing the reverse:

  $x=a

  if (condition) then $x=b

I like this, I think it's easier to read.

I don't care that its not the most efficient way to do things, programming languages exist to make things easier for me.

    $x=a  
    if (condition) then $x=b

If you think of programs as logic then I see the lines above as a logical expression written in CNF conjunctive normal form, where each line is a clause.

whereas this...

    $x = condition ? b : a;

...is like a nested logical expression.

And breaking the program down into clauses makes it easier to understand, for humans and computers.

A proposed solution from the article:

> Use "if (condition()) { ifBlock(); } if (!condition()) { elseBlock(); }" -- assuming that ifBlock() can't possibly change the result of condition().

What?? That seems terrible to me - just asking for `condition` to be updated to be modifiable by `ifBlock`, or for some completely undebuggable race condition to occur.

My team has an ongoing style war about guard clauses:

  if foo:
    bar()
  baz()

  if foo:
    bar()
  else:
    baz()

Consensus seems to favor the first option but I prefer the second because it helps me keep in mind the conditions that lead me to `baz()`. Otherwise I have to scan up the whole function to figure out the restriction `!foo`.

That’s not a style issue, those are two different things.

If foo is true, bar() and baz() are both executed in your first scenario. Only bar() is executed in the second.

Oh whoops you're right, Friday brain. The first conditional should have a `return` in it.