My language of choice is Javascript and I have been going through a legacy program and refactoring it. One of the ways I’ve been approaching the refactor is to attack a single code-smell at a time. My latest one is returning Boolean literals. It drives me nuts to see something like this:
function isFunction(fn) {
if (typeof fn === 'Function') {
return true;
} return false;
}
This is a contrived example (though not much more contrived than some of the functions I’ve cleaned up) but it serves my purposes here. The equality operator, ===, already returns a Boolean value, so we don’t need to specify it a line later. We can, instead, return the expression itself:
function isFunction(fn) {
return typeof fn === 'Function';
}A case that I’ve come across often is returning a Boolean out of a for-loop (for-loops are another code-smell in my opinion, but a subject for a different time). Here is the code in question:
function isAllUnderTen(numbers) {
for (let i = 0; i < numbers.length; i++) {
if (numbers[i] >= 10) {
return false;
}
} return true;
}
This is the reason that the Array methods some and every exist. These two functions return Booleans, so we can rewrite this as:
function isAllUnderTen(numbers) {
return numbers.every(n => n < 10);
}So why do I consider returning Boolean literals a code-smell?
- It’s too verbose.
- Boolean switching can happen inadvertently, either in writing or in reading.
Reason #1 is a problem because returning the Boolean literal instead of the expression you used to determine it wastes space, both in the file and in the reader’s mind. The reader (probably Future You) has to read not only the expression, but the next line. This relates to reason #2: if the next line returns the opposite of the expression, confusion will ensue. For example:
function isCoolNumber(n) {
if (n % 2 === 0) {
return false;
} return true;
}
In this code, if the expression n % 2 === 0 is true, we return false. Did the author mean for that to happen? Was it a typo? Or is this how the code is supposed to work? This is especially troublesome in code that has little to no test coverage. As a reader of this code, you probably don’t know what the expected behavior should be. The expression to determine the Boolean return value and the actual return value should always agree. I like to do incremental changes when refactoring (code does not leap out of my head fully formed, unfortunately), so I would change this to:
function isCoolNumber(n) {
if (n % 2 !== 0) {
return true;
}
return false;
}And then finish by changing it to:
function isCoolNumber(n) {
return n % 2 !== 0;
}One thing that I should mention before ending this article is that I consider returning implicit Booleans (truthy and falsey values) another code-smell. This is because you are not actually returning a Boolean, but something that can be coerced into a Boolean. This encourages off-api use of your function, which creates brittle code that will break if your function begins returning a real Boolean. For example, if isCoolNumber returned n % 2 instead (which would return the equivalently truthy/falsey values 1 and 0) then some enterprising developer may consume the function like, myNum + isCoolNumber(num), which would return NaN once isCoolNumber is refactored to return a real Boolean.
My preference now is to use the explicit equality and inequality (=== and !==) operators to do my evaluations but you could always wrap your expressions in double-negatives if it pleases you: !!(n % 2). It’s more terse, but at the cost of clear, explicit expectations, in my opinion.
