DEV Community

Daniel A. White
Daniel A. White

Posted on

Don't be afraid to expand your code for readability

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

Use long variable names

var flag = true; // :(

flag isn't very descriptive of what the boolean should be controlling. Don't be afraid to expand the name of variables to express what they really do. JavaScript minifiers' jobs are to shorten the variable names down - it isn't your job to do that - it's your job to write readable and maintainable code.

var shouldSaveDocument = true; // :)

Don't be afraid to break down conditionals

// :(
if (doc.hasChanged && !doc.isReadOnly && fs.hasFreeSpace && user.wantsToSave) {
    doc.save();
}

When doing some sort of precondition checking, don't throw them all into one if. The logical operators become a jumbled mess. It's hard to change or debug them especially when they are all on one line. Don't be afraid to break down conditionals and retest.

var shouldSaveDocument = true;

// retest the flag here so that operations can be added/removed easily and can see them clearly in a diff.
if (shouldSaveDocument && !doc.hasChanged) {
     trace('not saving unchanged document');
     shouldSaveDocument = false;
}

if (shouldSaveDocument && doc.isReadOnly) {
     trace('not saving read-only document');
     shouldSaveDocument = false;
}

if (shouldSaveDocument && !fs.hasFreeSpace) {
     trace('not saving document when there is not any freespace');
     shouldSaveDocument = false;
}

if (shouldSaveDocument && !user.wantsToSave) {
     trace('not saving document as the user does not want to');
     shouldSaveDocument = false;
}

if (shouldSaveDocument) {
    doc.save();
}

Discussion (17)

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 Author

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
lietux profile image
Janne "Lietu" Enberg

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 Author

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
lietux profile image
Janne "Lietu" Enberg

You could alternatively just use a good diff tool.

Collapse
lietux profile image
Janne "Lietu" Enberg

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