Recently, I changed some wording when I talk about code and code problems. In my opinion, the new words have more correlation with the root cause of the problem, while the old words used to be “nearer” to the symptoms.
Let me explain the changes with a small code example that serves no other purpose than to contain a few problems:
public class InMemoryItemRepository {
private final Map<String, Item> items;
public InMemoryItemRepository() {
this.items = new HashMap<>();
}
public Optional<Item> itemFor(String itemNumber) {
return Optional.of(this.items.get(itemNumber));
}
}
This class is a concrete implementation of an “item repository” that stores items by their “item number”. You can retrieve items by calling the itemFor method and giving it a valid itemNumber. Otherwise, you’ll end up with an empty Optional.
The first problem
The first problem in this code is a code smell. The data structure used to store the items is an HashMap. In Java, the HashMap implementation is not thread-safe:
Note that this implementation is not synchronized. If multiple threads access a hash map concurrently, and at least one of the threads modifies the map structurally, it must be synchronized externally.
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/HashMap.html
With our current implementation, we leak this limitation onto our clients, which will be totally unaware, because nothing in the class design hints towards an HashMap. In most production environments, our in-memory implementation might even be swapped out with a database-based one. And the database implementation hopefully is thread-safe.
A code smell is a bug
My change in wording calls this type of code problem a bug (instead of a code smell). It might be possible that right now, this class is only used in a single-threaded manner and the implementation flaw doesn’t matter. In that case, it’s a bug in hibernation. Right now, it sleeps, but the day will come when it wakes up. That doesn’t change its bug-like features, just its immediate damage potential.
If you label a code smell as a bug, you highlight the damage potential instead of the current “nice to have” prioritization.
The second problem
Let’s return to the code example. There is another problem with this implementation: If you ask for an item number that is not stored in the repository, you don’t get an empty Optional, you’ll get a nice and unexpected NullPointerException.
A small unit test that asks for any item number on an empty repository reveals the problem:
@Test
void returnsEmptyIfNotStored() {
var target = new InMemoryItemRepository();
assertThat(
target.itemFor("non-existent")
).isEmpty();
}
This test will run red with a NullPointerException, pointing to this line in the implementation:
return Optional.of(this.items.get(itemNumber));
Of course, this is “just a typo” in the way we construct the Optional. Because our HashMap returns null if a given key is not stored in it, we need to call Optional.ofNullable() and have it convert null to Optional.empty:
return Optional.ofNullable(this.items.get(itemNumber));
A bug is a missing test
This “little fix” makes our unit test pass and the repository useful. My change in wording calls every bug a “missing test”. This points out that the test you write as you fix the bug could have prevented the bug’s damage altogether.
If you were surprised by my assumption that you write tests when you fix bugs, please consider adopting this as a habit. It is the last possible moment to improve your test coverage at a place that has proven to be important enough to have tests and undertested enough to exhibit bugs. If you want your project to be antifragile (and there are good reasons why it should be), start by strengthening it at every place it breaks.
Tests for every code smell?
The combination of both changes would make every code smell a missing test. Right now, I’m not sure if this needs to be an automatism. The existence of a code smell hints towards a more fragile part of the system, but I’m not sure if “potential fragility” is a valid indicator for additional tests.
Of course, if you program completely test driven, these kind of thoughts probably seem pointless to you.
What are your thoughts on this topic and specifically on compulsory testing for every code smell?
Schneide Blog 