DEV Community

Discussion on: Don't be afraid to expand your code for readability

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