DEV Community

Don't be afraid to expand your code for readability

Daniel A. White on March 30, 2017

Variable names are cheap. Conditional checks are cheap. So why limit yourself when there's a cleaner way to express your code and increase its ma...
Read full post
Collapse
 
chaghdashko profile image
Cagdas •

Descriptive variable names are totally ok (and not a bit of code smell!). Code smells generally occur when you are clearly trying to wipe some junk (read 'using quick and dirty shortcuts' here) under the carpet. This is not the case here, and really helps self-documenting the code.

But, the refactoring of conditional statement is just confusing. Why?

First, we are checking the properties of three different actors with no visual hierarchy. Also, why do i have to see shouldSaveDocument check and shouldSaveDocument = false; statement everywhere? Lots of code replication.

Second, in the worst case, we are checking three more conditionals, which is totally unnecessary. This is just a simple example. But, using this kind of vertical expansion with more complex logical expressions could really create a mess.

In the ideal case, i would not refactor that conditional check. But, if i really want it, i would refactor like this:

if (shouldSave(doc))
    doc.save();
Enter fullscreen mode Exit fullscreen mode

And the shouldSave() routine:

/* Omitted traces here */
function shouldSave(doc) {
    if (doc.hasChanged && !doc.isReadOnly) {
        if (fs.hasFreeSpace) {
            if (user.wantsToSave) {
                return true;
            }
        }
    }

    return false;
}
Enter fullscreen mode Exit fullscreen mode

Separating the complex conditional check from the main function helps simplifying the code, thus increasing readability.

Using hierarchical conditional checks separates the property checks of three different actors and places them in a clear order to eliminate unnecessary checks. This code is easier to grasp, cleaner for a diff tool, and performs better.

Another alternative is using immediate return statements:

/* Omitted traces here */
function shouldSave(doc) {
    if (!doc.hasChanged) {
        return false;
    }

    if (doc.isReadOnly) {
        return false;
    }

    if (!fs.hasFreeSpace) {
        return false;
    }

    if (!user.wantsToSave) {
        return false;
    }

    return true;
Enter fullscreen mode Exit fullscreen mode

While eliminating unnecessary checks and preserving performance, i think this code is uglier, imho. Because, it is too long and lacks hierarchy. But, if your sole intent is to be more friendly with a diff tool, this may be a better alternative to your version.

Collapse
 
kevinrstone711 profile image
Kevin R Stone •

Separating complex conditionals into their own function is a great choice. Once inside I think code format is less important than having tests.
Additionally, this format can work too. 

function shouldSave(doc) {
   return doc.hasChanged && 
          !doc.isReadOnly && 
          fs.hasFreeSpace && 
          user.wantsToSave;
}
Collapse
 
andrespineros profile image
Andrés Felipe Piñeros •

Right, and if you want to debug, just print the values of the variables inside the shouldSave(doc) function.

Collapse
 
lluismf profile image
Lluís Josep Martínez •

The first conditional is perfectly readable. The expanded version is not!

Collapse
 
daniel profile image
Daniel A. White •

This is just a condensed example. Imagine many different steps.

Collapse
 
kevinrstone711 profile image
Kevin R Stone •

If there are many more steps is the method perhaps doing too much?

It makes me wonder how context dependent some of these choices are.

Collapse
 
pbeekums profile image
Beekey Cheung •

A friend of mine had an interesting practice.
Instead of calling functions with boolean parameters:
myFunction(argument1, true)

he made a variable solely for documentation:

shouldThisBeTrue = true
myFunction(argument1, shouldThisBeTrue)
Enter fullscreen mode Exit fullscreen mode

Much quicker to read than relying on IDE hover hints or having to go to the function.

Collapse
 
bengeorge profile image
bengeorge •

The repetition of shouldSaveDocument && is very very unreadable for me. But the one liner is perfectly readable. Perhaps a different example might express your point better ?

Collapse
 
binaryforgeltd profile image
Bart Karalus •

Every now and then I happen to work with other devs who spend a significant amount of time "compressing" variable names all over the codebase. Why? Because it looks cool? Takes "less space"...? Never really figured it out.

Collapse
 
erebos-manannan profile image
Erebos Manannán •

If you're writing code in a different way to cater to your diff tool, you need to find a better diff tool.

Collapse
 
kevinrstone711 profile image
Kevin R Stone •

If we already had the best tools imaginable I would not even write code. I would simply wish my applications into being.

Collapse
 
daniel profile image
Daniel A. White •

I'm not catering to the diff tool. I'm just saying it makes it more clear. Inline diffs can be a challenge for any diff tool.

Collapse
 
dmerejkowsky profile image
Dimitri Merejkowsky •

Catering to the diff tool may be important. I give an example here on my blog

Thread Thread
 
erebos-manannan profile image
Erebos Manannán •

You could alternatively just use a good diff tool.

Collapse
 
erebos-manannan profile image
Erebos Manannán •

When you say "inline diffs" you are specifically catering to a tool already, one that only does inline diffs.

Collapse
 
fp2sec profile image
cd / •

Long variable names are a code smell. If your variable names are long or descriptive, chances are your code is monomorphic.

Collapse
 
alejandrosobko_67 profile image
Alejandro Sobko •

I think that is more readable the if with all conditions that 5 ifs.
But I understand the final opinion..