Imagine you've written the following hack to see which OS you're running.
OperatingSystem determine_os(uname: &str) {
if (uname.contains("Darwin")) {
return OperatingSystem.MacOS;
} else if (uname.contains("Linux")) {
return OperatingSystem.Linux;
} else {
return OperatingSystem.Unknown;
}
}
You send it for code review, and get the following comment.
Nit: you can remove the final
else
.
Unfortunately, a common code review comment. But does it hold up to scrutiny?
What does it look like after applying the suggestion?
OperatingSystem determine_os(uname: &str) {
if (uname.contains("Darwin")) {
return OperatingSystem.MacOS;
} else if (uname.contains("Linux")) {
return OperatingSystem.Linux;
}
return OperatingSystem.Unknown;
}
By omitting the final else
, we've broken the structural and visual alignment
of logic that belongs to the same category.
Indentation is a visual manifestation of logical structure. Things that are alike logically and structurally should reflect that visually.
This function is effectively a tri-branch conditional return. And the code
should show that as explicitly as possible. The else
logically groups the code
and encodes that meaning into it via structure. There's a cargo
cult against using else
anywhere. Don't fall
for that trap.
If you're still not convinced, consider this. Would you write a switch statement like this?
switch(color) {
case "red":
return Color.Red;
case "blue":
return Color.Blue;
}
return Color.Unknown;
Or would you write it like this?
switch(color) {
case "red":
return Color.Red;
case "blue":
return Color.Blue;
default:
return Color.Unknown;
}