RallyDev.com
Mon 12 Sep 2011
Clean Code – Mutable Local Variables Are Evil
Okay, they’re not always evil. Counters and other pseudo-locals are fine. Problems arise when mutable local variables are used as return values for functions/methods. Reassigning a local variable, even once, can have a dramatic negative impact on readability.
Anyone who has ever paired with me knows this type of method/function is anathema. Assume this is an instance method on UserStory.
public UserStory getTopMostParent() {
UserStory topMostParent = null;
if(getParent() == null) {
topMostParent = this;
} else {
topMostParent = getParent().getTopMostParent();
}
return topMostParent;
}
Often you’ll see methods written like this because someone’s first mentor or most influential university professor advocated the structured programming practice of “one return statement per function”.
Whatever the reason, it’s bullshit.
I’ve heard various arguments in favor of “one return statement per function” but none that were good. One oft-cited explanation is the mathematical provability of one return per blah blah.
I’ve said before that readability is the most important -ility, and I stick by it. Any other -ility (except testability) is bullshit (until it’s not).
The above function should always (ALWAYS) be written like this:
public UserStory getTopMostParent() {
if(getParent() == null) {
return this;
}
return getParent().getTopMostParent();
}
This is a dramatic improvement in readability. Do it. There is absolutely no reason not to. One could argue that the local return variable might be reassigned depending on other conditional expressions in the function. I’d tell you to compose your methods.
One last thing – that last method could potentially have an “else” around the return getParent().getTopMostParent(). Don’t do it. It’s the last statement in the method. The else is redundant. Putting it in an else block adds one more thing you need to read to understand the method.
Any decent prof would also have taught that tail-recursive methods can be optimized into seriously-efficient loops by the compiler. Thus your answer is doubly-good.
Amen.
Method Name + Understandable Return statements = good methods =)
You can avoid reassigning the variable while also sticking to one-return-per-method. Just make the variable ‘final’ and don’t assign a value when declaring it. The compiler will insist that the variable be assigned once and only once. This improves readability especially in more complex methods than your example.
@John – That approach has all the same readability penalties as actual mutable variables. Multiple code paths assigning to a variable of the same name, regardless of whether the variable is final, is exactly the problem the post is trying to address.
As far as more complex methods: I wrote a post about that
I disagree that it has “all the same readability penalties as actual mutable variables”. With finals, I can scan the code until I find the one assignment that could logically apply, and know that it is the only assignment that will have been applied. That removes the mental load of keeping each of multiple assignments in mind. Decomposition is nice, as long as there aren’t too many method bodies to keep track of and scroll between.
I think we’re going to have to agree to disagree. “Scanning the code” is exactly what I’m trying to avoid. A final variable is declared, assigned, and returned in 3 separate locations. That’s more than a quick return forces you to keep in your head.
By returning early, you can also immediately understand the flow of the method. When you see return, you don’t have to read any further. Even if you are assigning to a final, there could be other things going on in the method.