A simple tip for cleaner code

While working on a codebase that has a lot of legacy code I noticed a lot of functions written like this. For some reason, the older devs in the team preferred to write them this way, and much to my annoyance they still do.

        // Not so nice.

        function foo($bar)
        {

            if ($bar > 0) {
                return "YES";
            } else if ($bar == 0) {
                return "MAYBE"
            } else {
                return "NO";
            }
        }


        // Not so nice as well.

        function foo($bar)
        {
            $value = '';            

            if ($bar > 0) {
                $value = "YES";
            } else if ($bar == 0) {
                $value = "MAYBE"
            } else {
                $value = "NO";
            }

            return $value;
        }

In my opinion, that's too much noise for your brain, we can make it a lot more readable just by utilising early returns.

        // Very much nice, yes, yes, sexy.

        function foo($bar) 
        {
            if ($bar > 0) {
                return "YES";
            }

            if ($bar == 0) {
                return "MAYBE";
            }

            return "NO";    
        }   

Isn't it a lot cleaner now? Don't be a dinosaur, use early returns!

Comments

[JavaScript Joel]

For some reason the younger devs in the team prefer to write code this way ☝️, but the truly enlightened devs will write code like this:

const foo = bar =>
  bar > 0 ? 'YES'
  : bar === 0 ? 'MAYBE'
  : 'NO'

or like this:

import { always, cond, equals, gt, T } from 'ramda'

const foo = cond([
  [gt(0), always('YES')],
  [equals(0), always('MAYBE')],
  [T, always('NO')],
])

Use a single return!

p.s. also be careful of ageism ;)

[Sasha Blagojevic]

Looks like the simplicity of my example stands in the way of the message I'm trying to convey.

If we take the examples literally there is no issue, a single line ternary

   return $bar > 0 ? 'YES' : ($bar === 0 ? 'MAYBE' : NO)

But if we have code between the condition and the return statement, you cannot use a ternary oh enlightened one ;)

[JavaScript Joel]

Yes your example was a simple one, though simplicity is not a factor as any program can be expressed as a single expression, regardless of the simplicity. Therefore any example can be converted into a conditional ternary. Though it may is possible not look the cleanest or be the best solution.

But if we have code between the condition and the return statement, you cannot use a ternary oh enlightened one ;)

☝️ This statement can be proven false with this block of code:

const foo = bar =>
  bar > 0 ? (
    console.log('Execute some code here for YES'),
    'YES'
  ) : bar === 0 ? (
    console.log('Execute some code here for MAYBE'),
    'MAYBE'
  ) : (
    console.log('Execute some code here for NO'),
    'NO'
  )

To demonstrate my point that you can write any application as a single expression, you can read my article where I have written a fully functional Promise library, a fairly complex program, as a single function expression. dev.to/joelnet/challenge-program-w...

The good thing about using a ternary operator is that when the operator becomes complicated, it is a signal that your function needs to be broken down into smaller pieces. Where as with the if statement, you can easily add too many statements without receiving this signal and end up with a function that breaks the Single Responsibility rule.

• The Enlightened One

[Sasha Blagojevic]

Just because you can do something does not mean you should, have fun maintaining your library :)

[Aram Bayadyan]

Just because you shouldn't do something it doesn't mean you can't ;)

[Sasha Blagojevic]

True as well :D

[Jon Bristow]

I’m fine with the quick return for validation or a guard as long as it’s right at the beginning of a function.

I’d rather have one single exit point for my function though since having multiple outs adds to the chances that someone will add code between the two exits without realizing it.

A “better” solution:

# python 2 only. Cmp was taken from the code golfers in python 3.
def silly(bar):
    output = {"-1": "NO", "0": "MAYBE", "1": "YES"}
    return output[str(cmp(bar,0))]

# LATE EDIT FOR MAXIMUM UNREADABILITY:
def silly(bar):
    return {"-1": "NO", "0": "MAYBE", "1": "YES"}[str(cmp(bar,0))]

If your function is equivalent to a nested ternary, then that’s a good use of early return. Anything more complicated needs to have the contents of the If statements extracted into methods to more clearly show off your now ternary equivalent ifelse

[JavaScript Joel]

I think OP's original example uses the conditions == 0, > 0 and < 0, so a hash table wouldn't work for 2 or -2.

[Jon Bristow]

I think if you look closer, you'll see that my solution is equivalent in output to the OP's example.

Here's a nastier one (also with a py3 equivalent)

# python2
def really_silly(bar):
    return ["MAYBE","YES","NO"][cmp(bar,0)]

# python3
def extremely_silly(bar):
    return ["MAYBE","YES","NO"][(bar>0)-(bar<0)]

God, I love how awful python can get sometimes!

[borisozegovic]

Joelnet, those two are not as readable as last example from Sasa, they have a higher mental load.

But I agree with you, better style comes with age and experience. :)

[JavaScript Joel]

This is due to familiarity bias.

It's easier for you to understand OP's examples because you have been exposed to them.

It's like reading a new word for the first time. You have to sound out every letter. But as you see that word more often, you can understand it at the quickest glance.

This does not mean that word is harder than other words you have already learned. You just haven't become familiar with it yet.

Learning more words will only improve your mastery over the language. And who knows, there may come a time when that word will be the one that will perfectly convey your message.

Cheers!

[borisozegovic]

No, no, I use arrow functions daily, but my mantra is that shorter is not always better. For me, double ternary is not readable and has higher mental load and we don't have any advantage over simple 'if else'. Writing is cheap and quick, reading is not. :) You also must take in consideration other developers who might not prefer ternary and double ternary and they will not be happy reading this. :)

[Alain Van Hout]

It’s an incredibly important skill to be able to say something in a simple way. A large vocabulary is useful, but doesn’t guarantee (or can even decrease) clear communication.

[Angel Daniel Munoz Gonzalez]

while I personally go for the first one, I tend to be a little bit more explicit by doing the OP's last example, because I've worked on code bases where there's a lot of new devs and they tend to break (and some times don't even touch them by fear) too much these ternary/single return statements, while I don't think they are bad, some times I just care more about explicitness for the rest of the team/teams, on my personal projects tough you will find more ternary/single return samples :smile:

[Simon Holmes]

That second one with ramda is a prime example of garbage code from a developer proving theyve read some functional programming books / blog posts. Why would you ever do that as opposed to the nested ternary? Functional programming is great but the use of ramda is complete nonsense for that example.

[Sérgio Luigi Alves da Silva]

I have personal restrictions about ternaries within ternaries

[toby]

Possibly older devs write like this because other languages (mostly outside of web stack AFAIK) can consider that a single exit point is safer, same goes for making sure (generally speaking) any 'if' has an 'else'. For example see MISRA-C-2004.

[Sasha Blagojevic]

That makes sense, you're probably right.

[Carter Wickstrom]

It’s almost certainly this. It’s always good to question convention, but when all of the more experienced devs follow a pattern, there’s probably a reason for it. It’s unfortunate that none of the ones you work with were able to explain why, instead of just being annoyed.

[Alesis Manzano]

Option 1, one return at the end. Best for debugging.
Option 2: early returns. I consider valuable when you have a simple condition that could be evaluated at the beginning and the rest of conditions are more complex, so you can save some precious cpu time haha

[Ryan Silva]

I'm fine with any of those, just don't mix curly brace styles! :-P

[Sasha Blagojevic]

Ugh, another one of my pet peeves. :P

[Tiago Marques]

        function foo($bar)
        { //why?

            if (...) {
                ...
            }

        } //why?

When I started reading, I thought the problem was that the inconsistency of the placement of braces...

[Sasha Blagojevic]

Well this is actually according to PSR - php’s community style guide :D Control flow has same line braces, functions have next line braces.

[Tiago Marques]

One more thing for me to dislike PSR... I really don't like they new line rules.
But thank you for explain me!

[edA‑qa mort‑ora‑y]

What about:

        function foo($bar)
        {
            $value = '';            

            if ($bar > 0) {
                $value = "YES";
            } else if ($bar == 0) {
                $value = "MAYBE"
            } else {
                $value = "NO";
            }



            foo( $value );
            return $value;
        }

The simple addition of foo makes the second form entirely reasonable.

Without knowing the history of the code it's hard to say why it's ended up at the form it currently has. It's way too easy to look at things in isolation and find fault with them, or draw conclusions about what is clean/bad.

[Alain Van Hout]

Please extract the middle part as a separate function and resubmit your pull request ;-p :-).

[Grumpy and]

I spotted this over the weekend, and had a few thoughts, but didn't have time to chime in. Now that I have time, it looks like a lot of what I would have said has already been covered. So just a few quick thoughts that I hope will be helpful.

I've worked on various codebases with different styles. What's more "readable" or "clean" generally is more a function of what you're accustomed to looking at than any objective measurement. Codebases with a consistent style are far easier to work on than codebases that mix different styles, even if they're consistently ugly.

Focusing on stuff like bracing styles, if/else formatting, etc, is focusing on the smallest possible minutia of the codebase. Consider: If you could magically reformat all the code so that it's to your liking, what would you want to fix after that? Is reformatting the code actually stopping you from doing that next thing? (Spoiler: No.)

I totally feel your pain in having to look at code that you feel could/should be clearer/cleaner. Please know that I've been there. But when the more seasoned devs you work with all tell you the same thing, it's worth strongly considering their perspective.

It's said that safety regulations are written in blood. The preferences of senior devs are based on long years of bad experiences. Ignore them if you like, but don't doubt that you'll eventually learn the hard way.

[ComputerSmiths]

Well made point!

But isn't the whole point of a function to abstract a thing that's done frequently in order to avoid copying the code, and making it easier to modify it in one place should changes be necessary?

I'd say I don't care which way it's written, as long as the documentation is clear on what it does, the function name gives me a hint, and I can look at the code once, determine that it's appropriate for my use, and then (the best part!) completely forget about the implementation details.

So in some sense, all of these, and their counter-examples, and the obfuscated code submitted by other commenters, are fine because they get the job done.

And in some sense they are all undocumented FooFunctions that are going to add to the debugging load and the GettingJuniorDevsUpToSpeedOnTheCodeBase time.

And on the other tentacle, it's just a religious style war, or a hazing ritual for the JuniorDevs (unless there's a well documented, properly enforced, company-wide style guide). Bonus points for easy documentation updates so I cay say "I'm using this over here in IncrementalSolarPayback" so any changes to the function will know what might break.

I am not a proper Dev, so feel free to tell me I'm out of line... [But no, I don't buy the code efficiency argument, either throw more CPU at it or write it in assembly. 8*]

[dallgoot]

personally i'd go with the less conventionnal :

        function foo($bar) 
        {
            if ($bar > 0) return "YES";
            if ($bar == 0) return "MAYBE";
            return "NO";    
        }  

..somewhere between ternary and switch disposition, i find it clearer, obvious and fautless performance-wise.
The point of concern is when another dev replace the return with more operations defeating the "escape" purpose of "return" statement.
So i can understand that the "else" are somewhat preventing falling through.
However, as $bar cannot at the same time be 0 AND superior to 0 AND something else, i'd say it's safe.

In terms of control structure it calls for a switch, however many people reject this form :

function foo($bar)
{
    switch(true) {
        case ($bar > 0):  return "YES";
        case ($bar == 0): return "MAYBE";
        default:
            return "NO";
    }
}

[Eric Keyte]

You should probably push your boss to fire all those old “dinosaurs” who are writing garbage code, and just hire a bunch of juniors to come in and revolutionize everything.

I hope in another few years I’m lucky enough to get the axe. I wouldn’t want to hold my company back. Or maybe they should start refactoring now. I’ll quit on my own volition if they want to get our finance app reduced down to a single function expression.

[Sasha Blagojevic]

Wow such anger and bitterness in your response, that’s unhealthy for you. Maybe you should strive to improve and better yourself instead od thinking you know it all just because you’ve been there for 10 or whatever years. God forbid maybe even learn something new.