DevTips: Use early returns to avoid nested conditions

As a developper, you will encounter some patterns you should identify as code smells. Most of them have well known solutions. Today I want to talk about using early returns to avoid else statements and nested conditions.

Let's take a example. I need to call a server to know if a person already exists in my database. The function that makes the call also return a loading indicator so I can inform the user.

render() {
  const personToLookFor = 'Thierry'
  const [result, loading] = doesPersonExists(personToLookFor)

  if (!loading) {
    let message
    if (result) {
      message = `${personToLookFor} already exists.` 
    } else {
      message = `${personToLookFor} doesn't exist.`
    }
    return message
  } else {
    return 'Loading...'
  }
}

As you can see the nested conditions and if/else statements are hard to read. You don't really understand what does this piece of code at first glance. I bet you already run into this pattern before. Let's refactor this a bit to make it more maintainable!

If the call is still pending we can directly quit the function and display the loading indicator.

render() {
  const personToLookFor = 'Thierry'
  const [result, loading] = doesPersonExists(personToLookFor)

  if (loading) return 'Loading...'

  let message
  if (result) {
    message = `${personToLookFor} already exists.` 
  } else {
    message = `${personToLookFor} doesn't exist.`
  }
  return message
}

Isn't it a bit clearer? We can also get rid of the else statement by returning directly the message in the if statement.

render() {
  const personToLookFor = 'Thierry'
  const [result, loading] = doesPersonExists(personToLookFor)

  if (loading) return 'Loading...'

  if (result) {
    return `${personToLookFor} already exists.` 
  }
  return `${personToLookFor} doesn't exist.`
}

It also removes the necessity to have a message variable. You're set 🙌

Hope it will help you!

---

Feedback is appreciated 🙏 Please tweet me if you have any questions @YvonnickFrin!

Comments

[Thomas Junkツ]

I think we settle this at this point.

At least for now - because I have neither a lab nor the money to fund the necessary experiments. But perhaps someone in the community has the right people in their network backing one or the other side ;)

[Thomas Junkツ]

Regarding code complexity, you are totally right. But regarding reading complexity I think you underestimate the cognitive load which you are putting on the reader. I'll try to elaborate on what I mean

To fully grasp what the function is doing, you start reading top down.
You come to a halt at this line:

return loading ? loadingMessage() : existsMessage(result);

This is an easy to digest statement. I think we both agree on that. But - as I have written above - you are hiding simple stuff behind simple stuff.

If I ask you, new to the code

What does the function return

the answer at this point of reading is dependend on loading. And if I ask you further

What is the exact text?

you have to scan down the lines first to loadingMessage where you have to parse

loadingMessage() {
    return 'Loading...';
}

Which is - as I have said above - a simple function. But to understand what the characters are doing, you have to read and parse the first line loadingMessage which tells you something which you are not interested in (the name of a function) and besides that () indicate that this is a function and {} are used to indicate the scope of the function. As soon as you start reading the first line, you wander to the last line to fully grasp what this function is about. The middle line tells you »Oh, it's just about returning a constant. Never mind«

After your journey to the parts below the actual return statement - remember: this is where we came from - you have to jump back to the line where we branched off (even mentally).

The explanation of the second part of your ternary and the parsing of the second function I leave up to the reader.

So I think I have good reasons to claim that the reading flow and in turn the readability of this code is worse than necessary. Additionally we could experiment with fellow programmers and eye tracking how distracting the patterns are which the eyes have to take to understand this part of the code. Further I would claim not only that I have good reasons but perhaps could proove my point with experiments.

To repeat my point:
In general this is a good practice. It is of course for a reason that we do not write 1000 lines functions. And when we split those we pay the price of doing extra mental work - as in your case - to improve our reading experience, because we structure our code from the more abstract but easy to understand parts of the code down to the nitty gritty details which may not so easy to digest nor understand. So the win is there understandability and using mental ressources effectively.

Why isn't the general practice not good in this example:
Because I have to walk a mental extra mile without gaining anything. I am distracted, my eyeballs have to do extra work only to see, that you are returning a string. Which is trivial.

This is like putting a saucer on a napkin on plastic tablecloth. So when soup drips it drips at least not on the tablecloth which is easy to clean up.

Therefore I came to the conclusion

So for this example it is not the best refactoring choice

[Thomas Junkツ]

I find this solution obfuscates what it is doing. This is good practice going bad and making code worse.

• On the upside: I am a big fan of keeping things simple. And in order to keep things simple, I tend to write small chunks of code for each step a function is taking. But this is not a means in itself: Usually you try to get complexity out of your way. Instead of querying the connection pool for a valid database connection, opening a database connection, building a SQL statement, sending it to the database, getting the result unpacking it into objecs, I like to read User.find("Thierry"). So hiding the magic behind the repository pattern is okay.

• But on the other hand: This code isn't hiding complexity: the functions are doing basic things which is really not hard to understand. But encapsulating "simple" things in "simple" functions, instead of decreasing reading complexity you increase reading complexity.

So for this example it is not the best refactoring choice.

[Thomas Junkツ]

And then there is:

render() {
  const personToLookFor = 'Thierry'
  const [result, loading] = doesPersonExist(personToLookFor)

  if (loading) return 'Loading...'
  return result ? `${personToLookFor} already exists.`
                : `${personToLookFor} doesn't exist.`;
}

Besides: Your function doesPersonExist() should take personToLookFor as a parameter - unless it's magical aware of what you want. ;)

[🦁 Yvonnick FRIN]

Thank you for reporting it Thomas, I fixed it 👍

[MurrayVarey]

Completely agree! Otherwise you can end up with an Arrow Head -- code where the important stuff is (a) hidden in the middle of the function, and (b) to the far right of the screen.

[🦁 Yvonnick FRIN]

Thank you for your suggestion! I find your proposal a bit complex for this case. But there are good ideas if the codebase grow. I tend to refactor as the logic gets more complex than planning evolutions that might not comes.

[Stephen Irving]

What about using ternaries so you only need a single return statement instead of three? More DRY, less code, as well as being faster to scan, read, and understand in my opinion. Should never have more than one return statement for a function if it can be easily avoided.

render() {
  const personToLookFor = 'Thierry',
        [result, loading] = doesPersonExists();

  return (
    loading
      ? 'Loading...'
      : `${personToLookFor} ${result ? 'already exists' : 'doesn\'t exist.'}.` 
  );
}
```

[Thomas Junkツ]

This ( »using ternaries« ) is in principle a good idea (I just posted my example and saw yours just now).

But have in mind that every branch adds more reading complexity.

If you use a ternary as a short form for when then it's from my POV okay - you are dealing with a binary outcome.

But when you nesting ternaries, you end up with

when then when then

which is harder to digest - which I tried to visualize by verbalizing it this way ;)

As a developer, I want to read as less code as necessary to understand what is going on. So your solution optimizes for that.

OTOH as a developer, I want to reason as less as possible as what a code is doing in case it has bugs. Taking that into account, I find your solution less favourable.

P.S.:

Or to contrast it with my own solution: I choose a similar approach like yours but added the guarding if-clause as a visual indicator of This is the uninteresting part. Nothing to see here. And the evalutation of the result which is from its nature a binary result (unless we are maybe at the quantum level) we have or we have not knowledge of a personToLookFor. And this binary result is explicitely written down as returning the result with the help of ternaries.

[Eljay-Adobe]

A few decades ago, structured programming was all the rage.

One of the things it advocated was a single return at the end of a routine. That made flow charts look good. Flow charts were also all the rage.

Some programming languages were designed during that era that only supported the single return at the end of a routine idiom.

Professors at colleges around the world were teaching the merits of structured programming, and many developers to this day still strongly abide by the single return at the end of a routine idiom.

Now we have object-oriented programming, and throwing exceptions. Exceptions are an invisible goto, and are another kind of a return. Violating the single return at the end of the routine idiom. Yet the strong adherents to structured programming turn a blind eye to exceptions in object-oriented land, because those are for exceptional situations and not normally happy path flow control. (No one would use exceptions for normal situation flow control, right? Mwa-ha-ha-ha-ha-ha!)

I give a glossy overview of the backstory not to promote single return at the end of a routine idiom. (I don't have strong feelings about it, pro or con.) Rather, to help people understand why many developers abide by that pattern.

Joel on Software, regarding exceptions:

• 13 (exceptions)
• Making Wrong Code Look Wrong

Raymond Chen, regarding exceptions:

• Cleaner, more elegant, and harder to recognize

[🦁 Yvonnick FRIN]

It is always good to know history. Patterns are here to help us developing. We don't have to follow them blindly. Thank you for sharing!

[Nick Holmes]

Nice article. Building an intuition for overly complex code is really essential. Often when working on code, there comes that moment when you hack things around to get things working. Its essential to refactor until its been simplified.

Some tool chains can calculate code metrics for you, and Cyclomatic Complexity is very relevant here.

Finally, some languages have support for pattern matching, which can really improve things here. Sorry, I'm not current on Javascript, but your example could be expressed in C# 8 like this;

string render() {
    string personToLookFor = "Thierry";
    (var result, var loading) = doesPersonExists();

    return (result, loading) switch {
        (_, true) => "Loading...",
        (true, _) => $"{personToLookFor} already exists.",
        (_, _)    => $"{personToLookFor} doesn't exist."
    };
}

(You don't strictly need to unpack the tuple, but its more readable)

[🦁 Yvonnick FRIN]

Thank you! Really interesting inputs in your comment. Thank you for sharing 🙏

[Niko Heikkilä]

Pattern matching can be (sort of) simulated in languages supporting switch cases. I find it very readable.

const render = (person = 'Thierry') => {
    const [result, loading] = doesPersonExists(person)

    switch (true) {
        case loading: return "Loading..."
        case result: return `${person} already exists.`
        default: return `${person} doesn't exist.`
    }
}