DEV Community

Cover image for If/else or just if?
Tyler Warnock
Tyler Warnock

Posted on

If/else or just if?

Today in a code review we had a statement like this:

Statement A

if (typeof val === 'string') {
    return 'A'
} else if (val === null || val === undefined) {
    return 'B'
} else {
    return val
}
Enter fullscreen mode Exit fullscreen mode

And a suggestion was made to switch to the following:

Statement B

if (typeof val === 'string') return 'A'
if (val === null || val === undefined) return 'B'
return val
Enter fullscreen mode Exit fullscreen mode

I won't tell you where we came out 😜, but which do you feel is a better way?

Was the suggestion to be concise and avoid if/else logic a good one, or was the original way better?

Top comments (88)

Collapse
 
somedood profile image
Basti Ortiz • Edited

I prefer the second syntax because it acts like a "guard" against further execution, so to speak. The first syntax just encourages more nesting and levels of indentations. To me, it is more readable to have a "guard" condition than to have if-else blocks.

Collapse
 
tyrw profile image
Tyler Warnock

I hadn't encountered the term "guard" for this but I like it a lot 👌

Collapse
 
somedood profile image
Basti Ortiz

I can't take the credit for it. 😉

Collapse
 
immathanr profile image
Mathan raj

I call it the Short-circuit or (Fast-fail)[en.m.wikipedia.org/wiki/Fail-fast]. It will help you just worry about less things if it's not followed by else

Collapse
 
dexterstpierre profile image
Dexter St-Pierre

I have the same train of thought with it! A senior developer at a previous job introduced it to me

Collapse
 
pedrohba1 profile image
Pedro Bufulin

The first time I heard the term "guard" was in haskell. Is it any related?

Collapse
 
somedood profile image
Basti Ortiz

I'm quite surprised actually. Given the imperative nature of if statements, I wouldn't expect Haskell (of all languages) to be the origin of that term.

Thread Thread
 
fennecdjay profile image
Jérémie Astor

Haskell (among other functional languages, I guess) has guards for pattern matching

sign x |  x >  0        =   1
       |  x == 0        =   0
       |  x <  0        =  -1
Collapse
 
sunnysingh profile image
Sunny Singh

I always go with Statement B. This is known as "early returns" and helps with confusing success/error cases. Take for example:

function createPost(post) {
  // Earliest potential error if no post is provided.
  if (!post) return false;

  // Ensure correct data type.
  if (typeof post !== 'object') return false;

  // Ensure required data is provided.
  if (!post.title || !post.body) return false;

  // We're all good at this point. Assume it's safe to create the post!
  const isPostCreated = create(post);

  return isPostCreated;
}
Enter fullscreen mode Exit fullscreen mode

It's nice to know for sure that the end of the function where we're saving the post to the database will not error out due to bad data, since we already verified those cases earlier.

Collapse
 
scooby359 profile image
Chris

I like this style more - it looks clearer to me, and from the point of working through the logic, I like that the returns just end the function.

If a block just set a value and returned it later, there's a chance it could be reassigned in another buggy block, so I think there's less risk in this style too.

Collapse
 
tyrw profile image
Tyler Warnock

This looks great to me. I'd be happy even without the comments as it feels pretty self-documenting.

Collapse
 
sunnysingh profile image
Sunny Singh

I added the comments just for the sake of the example, but they should definitely be removed for a real app.

Collapse
 
antogarand profile image
Antony Garand

I prefer the first approach, it's ensures all cases are covered, and is more extensible IMO.

I also prefer a single return per function, excluding sometimes null / wrong validations, as this code tends to be easier to read and debug. In your example, I would use the first method, but assign the return value to a variable, and return it after the statements.

This made me thing of this post on hackernews: Please do not attempt to simplify this code

Kubernetes controller source

  1. Every 'if' statement has a matching 'else' (exception: simple error checks for a client API call)
  2. Things that may seem obvious are commented explicitly

We call this style 'space shuttle style'. Space shuttle style is meant to
ensure that every branch and condition is considered and accounted for -
the same way code is written at NASA for applications like the space
shuttle.

Collapse
 
tyrw profile image
Tyler Warnock

Can you elaborate on the "cases are covered" point? In my mind both snippets covered all cases, but I'm open to hearing otherwise.

Collapse
 
antogarand profile image
Antony Garand

In this case, both examples are the exact same of course.

This is somewhat related to a single return statement in a function, but the syntax of an if/else if/else is more explicit than the various conditions are related to each other.

With few if statements each statement is independent from the others (unless you return in the previous cases, like in your example), therefore more than one of them can be triggered.

Overall, while there aren't many arguments for any of these approach in your specific scenario, I feel like it's a better practice to have a single style of conditions for your whole codebase. As the first approach is more flexible, I strongly prefer using it everywhere!

Thread Thread
 
sunnysingh profile image
Sunny Singh

Consistency is definitely most important. I just did a search through an API that I built recently, and I only have 3 else statements which I can probably refactor out. So, I think it's definitely possible to go all-in on early returns.

Collapse
 
sunnysingh profile image
Sunny Singh

TIL about "space shuttle style", very interesting.

I'm not sure if the Kubernetes controller source is meant to be used as an example though. Otherwise there would be no comment to warn people from simplifying the code.

Collapse
 
simbo1905 profile image
Simon Massey • Edited

In other languages such as Scala and Rust there is a "match" statement and a compiler that looks at the types can tell you if all cases are matched. In Scala you can compose/chain together sets of partial methods that match a set of related cases and the compiler will tell you if the combination will do an exhaustive match. So what the Kubernetes example you post is pointing out is that the devs are having to work around a missing feature in the language they are using. Once you have used a language that has an exhaustive match you never want to go back. This is because it is very much easier to write code that you know won't just fall through to some wrong default case when someone adds a new type of input.

Collapse
 
wuz profile image
Conlin Durbin

I prefer B, if only because it is a one to one mapping of cases to output - when reading the function it says:

When function is called
If val is a string, then return 'A'
If val is null or undefined, then return 'B'
Return val otherwise
Enter fullscreen mode Exit fullscreen mode

This is a much more human way of thinking about the possibilities. I don't tend to think in terms of if/else conditions in the real world and I don't feel like much business logic works that way either. Option B is also easier to extend - if later you need to add an "if val is a number, return val.toString" or the like, then that is easy and you can add it wherever it makes sense in your code.

Slightly unrelated, but I also really perfer this style to switch statements. They have a place, but most times they should probably be multiple if statements.

Collapse
 
tyrw profile image
Tyler Warnock

@wuz Well said, and I totally agree on the switch statements piece. I'm not sure I've ever been satisfied with the look/readability of a switch statement in JS!

Collapse
 
coachmatt_io profile image
Matt

I don't prefer one over the other, but what I do prefer is a single return statement, so you can make more assumptions about the code (eg: should get to the end of the function if there's no error)

let result

if (typeof val === 'string') {
    result = 'A'
} else if (val === null || val === undefined) {
    result = 'B'
} else {
    result = val
}

return result
Collapse
 
maixuanhan profile image
Han Mai

IMO, the single return comes from C coding guidelines where it may help developers easier to trace the complex code and prevent them from forgetting release any resources which need finalizing (like freeing heap). For this point, C++ projects usually don't adopt this guideline since it has its own stack-unwinding thing.

For Javascript, I prefer the statement B for more readable code. Save everyone's effort.

Collapse
 
fhusquinet profile image
Florian Husquinet

I find it more confusing to return a variable in a return unless it is really basic (like changing the message of a JSON response depending the data given).

Early returns make it sure you don't even up with mutating the response somewhere later in the code. It also make more readable (imo) because not everything is nested.

Collapse
 
simbo1905 profile image
Simon Massey

A lot of people would recommend using const by default. A variable that can be many things is cognitive overhead and results in bugs down the line.

Collapse
 
farble1670 profile image
Jeffrey Blattman • Edited

I prefer the "return early" syntax. It puts a line in the code where if you pass it, you know some assertion holds. The code above doesn't really illustrate that, but for example:

if (flag1) {
  return;
}
// Anything that follows, regardless of if / else nesting
// I know that flag1 is false.

As opposed to:

if (flag1) {
  return;
} else {
  // some stuff
}
// Some more stuff. Is flag1 false?

In the second example, imagine some ugly nesting. For each block of code you need to think "am I in a level where I've tested flag1?"

Collapse
 
tyrw profile image
Tyler Warnock

@farble1670 so do you prefer a hybrid of first and second? Something like:

if (typeof val === 'string') {
    return 'A'
}
if (val === null || val === undefined) {
    return 'B'
}
return val
Collapse
 
gypsydave5 profile image
David Wickes • Edited

Another option (for those who favour expressions over statements) can be built out of JavaScript's ternary operator:

return (typeof val === 'string') ? 'A'
     : (val === null || val === undefined) ? 'B'
     : val

Quite a popular pattern.

Collapse
 
tarialfaro profile image
Tari R. Alfaro

Ternary operators are really nice for simple and small statements, but I definitely wouldn't recommend them for anything complex. Makes things to hard to read for me. The same could be said for other developers.

Collapse
 
byrro profile image
Renato Byrro

That's not easy to read at all in my opinion...

Collapse
 
qm3ster profile image
Mihail Malo • Edited

If we know the branches will have return, then B, plain if, aka early return is better.

We don't need to keep the context of whose else we are in anymore most of the time.

Even refactoring it is easier, both in terms of just editing the code and the resulting git diff.

However, if we don't have return...

let val
// ...
if (typeof val === 'string') {
    val = 'A'
} else if (val === null || val === undefined) {
    val = 'B'
} else {
    val = /* do something here lol */ val
}
// ...
use(val)

...then there are two options for me.

1. Extract into function

function testVal(val){
  if (typeof val === 'string') return 'A'
  if (val == null) return 'B'
  return val
}
// elsewhere...
let val
// ...
val = testVal(val)
// ...
use(val)

2. Use a conditional expression

let val
// ...
val =   
  typeof val === 'string'
  ? 'A'
  : val == null
    ? 'B'
    : val
// ...
use(val)

It just depends on the complexity, relevance, and size

What's particularly good about both?
They end up only assigning val once, so after some further refactoring we may end up with the superior const binding!

F.A.Q:

Is early return at odds with functional programming?
Should we strive to use conditional expressions at every size?

No. Used correctly, return is just guards in a match expression. You could see it as advanced pattern matching that is missing from JS syntax.
(Early throw, on the other hand, is usually a hack we have to use for lack of static types.)

Collapse
 
ryansmith profile image
Ryan Smith • Edited

I prefer Statement A since it is more familiar, more explicit, and groups the possible outcomes. Almost every programmer has seen and used option A. It doesn't require changing the structure to make an update, if something needs to be added before the return statements, it is easy and predictable.

The second statement seems like brevity for the sake of brevity to me. It is shorter, but I'd argue that it is not concise because it does not present the information clearly (at least to me). On Statement B, the formatting would trigger a red flag for me. I'd likely have to take a second or third look just to make sure I know what the intention of the code was. I don't like single line if-statements because all it takes is accidental reformatting without adding braces to cause issues that are easy to miss. Some will argue that their editor does this all for them so maintainability is not an issue, but when working on a team I don't think that is a safe assumption. I'm sure someone could also argue efficiency gains of option B (smaller JS file), but that seems like overkill for minimal gain.

Collapse
 
moopet profile image
Ben Sinclair

I think the second example is muddying the water by combining the pattern (failing early) with a style choice (single-line, braceless ifs).

I prefer Statement B, or would if it was written like this:

if (typeof val === 'string') {
  return 'A';
}

if (val === null || val === undefined) {
  return 'B';
}

return val;

I like this because it reduces nesting and acts as a barrier to people over-complicating a function. I mean that if the function ends up having to re-use a negated form of one of the earlier conditions or have a separate set of nested ifs after the guard conditions, then it's a big smelly smell and should be moved to its own function instead.

Also, if you think about it in terms of human experience, it's how we work. If you're tending a patch of garden, you don't think about what category of flower you're looking at and cross-reference it in a book if it's obviously a weed.

Collapse
 
nektro profile image
Meghan (she/her)

The second one. Every time. For the early returns reason mentioned earlier. Since you're doing a return, the else is an anti-pattern.

Also this thread terrifies me for all the code that exists out there and solidifies everything about xkcd.com/2030/

Collapse
 
simbo1905 profile image
Simon Massey • Edited

Totally! "else" at the end of an if/else chain is a trap awaiting someone changing the input then making a mistake in the if/else such that the new input falls into the old else that does the wrong thing. Scala has "match" and the compiler gives an error if you don't match everything that can come in. Putting in a wildcard match at the end, which is equivalent of a last 'else' in JS, reads as "okay I don't recognise this input so I am going to ignore it". That screams anti-pattern as being both inefficient and inexact.