I was recently working on some unfamiliar code and ran across a confusing bug. I’ve changed the names and nature of the code below, but the essence of the bug remains.
One of the model classes defines a constant that is an array containing some symbols. Other methods in the class take an optional parameter containing a subset of these symbols; if the parameter isn’t specified, then it defaults to the constant.
Using this code, you can get a list of characters for a subset of the families:
The problem came when some other code used the
FAMILIES constant to
compute the subset to pass to this method. There are a number of
layers of method calls involved, so the bug was obscure, but
essentially there was some code doing this:
It looks like there is a rename from
happening for some reason. All of the other evidence in the code
suggests that there was a rename the other way at some point. We’re
not yet sure what to make of this.
Even if we assume that this rename makes sense, the problem is that the code is mutating an array that we thought was a constant. There is nothing in Ruby that stops anyone from modifying the contents of the array as long as they don’t try to reassign the constant itself. The same issue would arise if the constant was an instance of any other class that had methods allowing the internal state of the object to be changed.
There are several code smells here:
The replacing of
:rockheadsis questionable. This codebase is new to us, so we haven’t yet figured out if it makes sense to do this. There’s a comment nearby that suggests that it is intentional; we’re taking that at face value until we learn more.
Mutating the array in-place is likely not the best idea; sometimes we want to avoid creating extra garbage, though. Either way, it’s a good idea to know where the array came from before mutating like this.
Direct access to the
FAMILIESconstant breaks encapsulation.
freeze the array when creating it. That would stop any
in-place mutation, but we’d have be sure that our tests exercise every
code path that might try such a mutation. Since the code is still
pretty new to us, we’re not sure how thorough the tests are.
We could have changed the code that was mutating the array; using
freeze would have forced us to do this. There are at least two
places where some kind of mutation is going on; there might be more,
and we can’t be sure that no one else will ever implement something
similar, so we rejected this approach.
We ultimately decided that the broken encapsulation was the most important problem to address. This fixes the problem at the source so that no other code accesses the constant directly. Any indirect accesses get a copy of the array, not the original.
We also decided to assume (for now) that the rename from
:rockheads is legitimate, so we added a clause to our
statement until we better understand the reason for it.
This fixes the immediate bug, creating a safer world for us to continue to learn the codebase and make improvements. We could go one step further and make the constant private; we haven’t (yet) done that in this case.
In general, I like to find a way to fix bugs like this by making it so that it is impossible for the bug to occur. I don’t want to have to trust other programmers, including my future self, to remember the intimate details of how my code needs to be used.
Instead, my code should only allow proper usage. If I can’t quite reach that ideal, then it should communicate proper usage as best it can and, if necessary, protect itself from improper usage.