DEV Community

DevTips: Use early returns to avoid nested conditions

🦁 Yvonnick FRIN on December 12, 2019

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 ...

Read full post

John Oerter • Dec 12 '19

Thanks for posting this! This is one of my favorite methods of cleaning up hard to read code. I think it makes a big difference in understanding.

🦁 Yvonnick FRIN • Dec 12 '19

Thank you John! What are your other favorite methods? I'm curious.

John Oerter • Dec 12 '19 • Edited

My second most used method is splitting up code into separate functions/methods. And in C# or other OOP languages, I make heavy use of interfaces and separate classes. I practice TDD, and this really helps me see where to split functionality for maximum readability.

Another trick I use quite often is moving conditional expressions into methods/functions. So instead of:

if (score === 10) {
  // do something with a strike
}

I prefer to say:

if (isStrike(score)) {
  // do something for strike
}

I think this really takes a mental load off when other developers are trying to understand the conditional. What about you?

🦁 Yvonnick FRIN • Dec 12 '19 • Edited

TDD really makes the difference. I think code don't need to be perfect. It only needs to follow a few statements:

Single responsibility principle (It is related to what you said about splitting code into different functions)
Avoid over engineering (I prefer copying a same code at least three times before thinking about making it generic)
Keep code simple so beginners can understand it easily (I take care to avoid tricks like + before a variable to cast it in Number for example)

In my humble opinion following these help keeping a codebase maintainable 👌

Yann Bertrand • Dec 12 '19

Totally agree!

I also try to instantiate boolean variables instead of multiplying conditions inside an if statement.

const userIsLoggedIn = user.isLoggedIn()
const userCanComment = user.score > 10

if (userIsLoggedIn && userCanComment) {
  // Comment
}

The code speaks by itself!

John Oerter • Dec 12 '19

Yes! It's so easy to do and is a huge win for readability

Comment marked as low quality/non-constructive by the community. View Code of Conduct

Stephen Irving • Dec 12 '19 • Edited

If a professional JavaScript developer isn't aware that using the + operator before a string that represents a number will cast it to a number, and doesn't just look it up the first time they see it so that they understand it for all future times, then they shouldn't be a developer at all. Full stop. Thats not a "trick", it is a basic feature of the language and at the moment it is both the most performant and shortest way to cast a string to a number. Why would you choose an objectively worse method to do something just so if some idiot, who doesn't understand the language and is unwilling to learn anything new, comes along they will be more able to understand it. With all due respect, that is an absolutely ridiculous precept to follow.

It is also just wrong when you say "I copy code three times before I think about making it generic." Wrong, wrong, wrong. DRY is probably the number one precept of programming to follow that they teach you in every 101 level CS course. Not repeating yourself is not "over-engineering", it is a basic practice that every programmer should be following 100% of the time. Why wait til you've repeated yourself just three times? Why not 5? 10? 100? What is it about the number three that makes that the cut-off point where you decide you've repeated yourself enough? The common refrain isn't "Don't Repeat Yourself But It's OK If You Do So Three Times".

🦁 Yvonnick FRIN • Dec 12 '19

I don't like your tone. I find it really offensive for learning developers. Despite it I will answer for other people reading it.

In teams you find really different kinds of developers (beginners, seniors, lead, ...). The codebase isn't only for experts. I prefer having simple code that anyone can read regardless of their level on the language or framework used on the project. This is healthier for the project life in my humble opinion.

About the "rule" of three times. I prefer see a code used in different situation before make it generic so my conception is driven by real usage rather than planning every use case possible even if I don't need it at the end which is a common mistake I encounter.

You talk a lot patterns but they are just a way to handle problematics. They aren't always the solution. Sometimes a context need you to act differently for exemple on my current job I have to lead a team of beginners and help them learning JavaScript as we develop an app. I prefer making simple code everyone on the team understand rather than making the perfect code or follow every programming idioms.

Arthur • Dec 13 '19

I keep seeing seniors advising other devs to change jobs and forget the idea of them being real devs. It really needs to stop, people require time to learn and if you nurture you juniors properly, you'll have the strongest team ever. This guy sounds like the type of person who'd laugh in the face of someone who's made a mistake and put them down for this. Can't envy his colleagues unless it's a viper nest filled with the Elite just like him.

Thomas Junkツ • Dec 13 '19

@Yann Bertrand

Although, I know you just posted your code as an example and although I know it is really nitpicky from me, but when looking at your code:

const userIsLoggedIn = user.isLoggedIn()
const userCanComment = user.score > 10

if (userIsLoggedIn && userCanComment) {
  // Comment
}

I see two things:

you have a method isLoggedIn() on user. From my POV user.isLoggedIn() is equivalent to userIsLoggedIn. This is no win.
userCanComment is really better than user.score > 10 which is really bad. But, why is this no method on the user too?

So you would end up with

if (user.isLoggedIn() && user.canComment()) {
  // Comment
}

and no extra variables needed.

Ben Sinclair • Dec 13 '19

@stephenmirving

then they shouldn't be a developer at all

That's a really unhelpful thing to say.

Pretending it's not... I'm a professional developer who has used Javascript on and off since it was first released (though it's not my primary language by any means). I wouldn't use a hack like that, because I think it's awkward and likely to cause confusion or problems farther down the line.

There are prefectly valid ways to do things that don't sacrifice readability; when someone tries to refactor it out later on because it seemingly does nothing, they'll be at fault for introducing a bug. I say the original author introduced the bug, and would pick that up in a code review.

Yann Bertrand • Dec 13 '19

Agreed! I was not inspired, thanks for going further 🙂

Antoine Jouan • Dec 24 '19

I am agreed with your improvement expect for adding predicate to user class.
This will break Single Reponsability principle.
I think it's better to make a class for predicate which will manage specification and one class for user which will manage user state.
So if you want to keep this syntax then you can set predicate as extension of user class but in js it's little tricky and not simple for beginner.

Thomas Junkツ • Dec 25 '19

I do not quite understand. Starting with "classes". You could do that without classes at all. Plain objects will do.

Nick Holmes • Dec 12 '19

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 • Dec 12 '19

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

Niko Heikkilä • Dec 12 '19 • Edited

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.`
    }
}

Thomas Junkツ • Dec 13 '19 • Edited

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.

🦁 Yvonnick FRIN • Dec 12 '19 • Edited

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 • Dec 12 '19 • Edited

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ツ • Dec 13 '19 • Edited

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.

Thomas Junkツ • Dec 13 '19 • Edited

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 • Dec 13 '19 • Edited

Thank you for reporting it Thomas, I fixed it 👍

Thomas Junkツ • Dec 13 '19 • Edited

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

Eljay-Adobe • Dec 12 '19

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:

Raymond Chen, regarding exceptions:

Cleaner, more elegant, and harder to recognize

🦁 Yvonnick FRIN • Dec 12 '19

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!

Thomas Junkツ • Dec 13 '19

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 ;)

MurrayVarey • Dec 12 '19

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.