3 minutes
Avoid Combining Booleans
I was writing some code for KotlinPoet and I was writing a function that decides whether kotlin.Unit
should be emitted
as a return type (applications can choose to omit Unit
as a return type but libraries should be explicit about what they’re
returning for compatibility). Here’s a shell:
/**
* Returns whether [Unit] should be emitted as the return type.
*
* [Unit] is emitted as return type on a function unless:
* - It's a constructor
* - It's a getter/setter on a property
* - It's an expression body
*/
private fun emitUnitReturnType(): Boolean {
}
We write code the way we think, so the logic for this function could be something like:
/**
* Returns whether [Unit] should be emitted as the return type.
*
* [Unit] is emitted as return type on a function unless:
* - It's a constructor
* - It's a getter/setter on a property
* - It's an expression body
*/
private fun emitUnitReturnType(): Boolean {
return !isConstructor &&
(name != GETTER && name != SETTER) &&
body.asExpressionBody() == null
}
This perfectly mirrors our thought process but this code is horrendous to review. I don’t know about you, but it will take me a minute to understand what’s happening here. If the kdoc wasn’t present, I’d be lost.
We can simplify this by pulling out a few variables:
/**
* Returns whether [Unit] should be emitted as the return type.
*
* [Unit] is emitted as return type on a function unless:
* - It's a constructor
* - It's a getter/setter on a property
* - It's an expression body
*/
private fun emitUnitReturnType(): Boolean {
val isGetterOrSetter = name == GETTER || name == SETTER
val isExpressionBody = body.asExpressionBody() != null
return !isConstructor && !isGetterOrSetter && !isExpressionBody
}
This is much better but software evolves. Maybe Kotlin changes their mind about when we should emit Unit
as return type.
In that case, we’d have to add another boolean to this combinatorial logic. If you work in feature code, I don’t have to remind
you about ever-evolving and changing business conditions.
Here’s a variation that reads much better:
private fun emitUnitReturnType(): Boolean {
if (isConstructor) {
return false
}
if (name == GETTER || name == SETTER) {
// Getter/setters don't emit return types
return false
}
return body.asExpressionBody() == null
}
By deconstructing each condition and returning early, this code is a lot easier to read. I also omitted the docs because now the code is self documenting.
And what if you need to add more conditions? That’s easy for you as a programmer (just another if statement) and easy for your reviewer since they’re only evaluating one extra statement.
As a code reviewer, I find combined booleans to be taxing. Do your code reviewer (and future self) a favor and break them down into multiple statements.