DEV Community

Sasa Blagojevic
Sasa Blagojevic

Posted on

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;
        }
Enter fullscreen mode Exit fullscreen mode

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";    
        }   

Enter fullscreen mode Exit fullscreen mode

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

Discussion (38)

Collapse
joelnet profile image
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'
Enter fullscreen mode Exit fullscreen mode

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')],
])
Enter fullscreen mode Exit fullscreen mode

Use a single return!

p.s. also be careful of ageism ;)

Collapse
blackcat_dev profile image
Sasa Blagojevic Author

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

Collapse
joelnet profile image
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

mic drop

Thread Thread
blackcat_dev profile image
Sasa Blagojevic Author • Edited on

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

Thread Thread
aramix profile image
Aram Bayadyan

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

Thread Thread
blackcat_dev profile image
Sasa Blagojevic Author

True as well :D

Collapse
jbristow profile image
Jon Bristow • Edited on

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

Thread Thread
joelnet profile image
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.

Thread Thread
jbristow profile image
Jon Bristow • Edited on

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!

Collapse
borisozegovic profile image
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. :)

Collapse
joelnet profile image
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!

Thread Thread
borisozegovic profile image
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. :)

Thread Thread
alainvanhout profile image
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.

Collapse
tunaxor profile image
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:

Collapse
srsholmes profile image
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.

Collapse
sergioluigi profile image
Sérgio Luigi Alves da Silva

I have personal restrictions about ternaries within ternaries

Collapse
toby profile image
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.

Collapse
blackcat_dev profile image
Sasa Blagojevic Author

That makes sense, you're probably right.

Collapse
carterwickstrom profile image
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.

Collapse
alesisjoan profile image
Alesis Manzano • Edited on

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

Collapse
rysilva profile image
Ryan Silva

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

Collapse
blackcat_dev profile image
Sasa Blagojevic Author

Ugh, another one of my pet peeves. :P

Collapse
inozex profile image
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...

Collapse
blackcat_dev profile image
Sasa Blagojevic Author

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

Collapse
inozex profile image
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!

Collapse
mortoray profile image
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.

Collapse
alainvanhout profile image
Alain Van Hout

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

Collapse
craser profile image
Chris Raser

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.

Collapse
computersmiths profile image
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*]

Collapse
dallgoot profile image
dallgoot • Edited on

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";
    }
}
Collapse
ekeyte profile image
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.

Collapse
blackcat_dev profile image
Sasa Blagojevic Author • Edited on

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.

Collapse
whoisryosuke profile image
Ryosuke • Edited on

I tend to use the second example a lot in code that isn't large or repetitive enough to be abstracted, and I don't need to return a value immediately. That way I can also intercept the value before it's returned a couple times, and run other checks if necessary.

The final example is a little more elegant, but makes presumptions about the situation. Ideally we'd all do that, but then we'd also have one-off functions for logic that's singular in occurrence.

Collapse
dean profile image
Dean Bassett

I always try to avoid "else". I know Golang encourages the early return pattern

Collapse
bizzy237 profile image
Yury • Edited on

var dict = {'1':'YES',
'0':'MAYBE',
'-1':'NO'};

function foo($bar) {
    return dict[Math.sign($bar)];
}
Collapse
lucpattyn profile image
Mukit, Ataul

Returning from functions at will is against programming principles. The older programmers knew better.

Collapse
jonrandy profile image
Jon Randy

Both listings have early returns, it's just that the elses in the first listing aren't really needed

Collapse
blackcat_dev profile image
Sasa Blagojevic Author • Edited on

Then my wording was probably wrong, could you help me rephrase it? I could have probably gone with a better example, thanks for noticing. Added another "not so nice" example.