Daniel A. White

Posted on Mar 30, 2017

Don't be afraid to expand your code for readability

#conditionals #variables #namingthings

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

Top comments (17)

Cagdas • Apr 5 '17

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

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

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;

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.

Kevin R Stone • Oct 25 '17

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

Andrés Felipe Piñeros • Nov 5 '17

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

Lluís Josep Martínez • Mar 30 '17

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

Daniel A. White • Mar 30 '17

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

Kevin R Stone • Oct 25 '17

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.

Beekey Cheung • Apr 21 '17

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)

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

bengeorge • Mar 31 '17

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 ?

Bart Karalus • Oct 25 '17

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.