Bits of Bad Code #0

Welcome to the first in running series of articles titled Bits of Bad Code! I've always found failure to be a powerful learning mechanism. The ability to learn from the failure of others is an important part of that. This article series stems from a habit a built up with some of my teams to share example of bad code we've found and talk about why they are bad and how to make them better. This has been a fun experience for a my teams and a wonderful learning experience for growing software engineers. The goal of this series is to share similar examples of bad code and talk about how to make them better.

The Bad Code

This example is based on a real-world code snippet I encountered on a large team. While the code itself isn't incorrect or broken in any way, it's longer and more complicated than in needs to be. The goal of the function was to get the result of some other function call and if the result was true, return true. Otherwise, return false.

function foo() {
    var result = isSomeFlagTrue();
    if (result == true) {
      return true;
    }
    else {
      return false;
    }
}

The problem here is just that this is more logic than we need. This seven line snippet can be reduced to a shorter 1-line function that is both faster and consumes less memory. And, while speed and memory aren't important on a function of this scale, when they pop up all over a large codebase, the impacts can be non-negligible.

Better Code

We'll improve this in two steps. In the first, let's remove the variable declaration by calling the isSomeFlagTrue() function in the if condition. The other change we've made is we're not comparing the result of the function to true, we just evaluate the return value which is itself a boolean value.

function foo() {
    if (isSomeFlagTrue()) {
      return true;
    }
    else {
      return false;
    }
}

This first set of changes eliminates a variable and simplifies our logic a bit. But there's one much larger logical change we can make. Since our if condition just looks at the true/false value returned from a function and returns true when true and false when false, we can simply return the result of the function.

function foo() {
    return isSomeFlagTrue();
}

There you have it! Our function has been reduced from a busy seven lines to a simple one-liner. It is worth considering these simplification carefully in more complex situations. Often times reducing a function to one line comes at the expense of making it harder to read and understand. That's not the case here, but keep it in mind before applying this concept everywhere.

Another thing to consider is whether or not this function is really needed at all. If it can be easily reduced to a one-liner, why do you have it? Why not just call the underlying function it returns? Too many one-liners can be an indication of too many layers in your function call stack. This can also make code hard to understand as you go look at a function and find that it calls another function which may call another and so on. If you have a lot of super short functions, take a step back and look at the bigger picture and ask if they are really needed.