An Illustrating Example
Take a look at the following code sample. What is the problem with it?
public Task getNextTaskWithoutUpdate() {
Task task = repository.getNextTask();
task.setStatus(TaskStatus.Processed);
repository.update(task);
return task;
}
So what happened here? Initially someone wrote code that processes tasks stored in a database. He then noticed that tasks are processed multiple times. So a quick fix was implemented and tasks are automatically set to processed when they are grabbed. So they are processed once only.
In order to save a little time the method was not renamed – calling code would have to be updated and more binaries would have to be recompiled. And after all: “the code works!” – Right?
So what is the problem with this? Later on the application is extended to inspect upcoming tasks and collect some statistics about upcoming tasks. The engineer implementing this functionality calls getNextTaskWithoutUpdate() to get the required data. Little does he know with this he has permanently removed the task from processing. Later the new application version is released to a customer. Several tasks go by unprocessed and the custommer incurs reputational and financial damage because of unprocessed tasks.
The Problem
While the immediate harm in the example above is bad enough, there is a much bigger problem with code like this. Add changes like this to your code base a few times only, and every reasonable engineer will lose trust in the code. There are now only two approaches to ensure any code he calls actually does what it claims to do:
- Read every single line of code called, and every single line of code called by code called, etc.
- Test every method called with thoroughly . Does it do what it claims? Are there any strange corner cases? Are there any unexpected side effects, e.g. database object changes?
The truth is, both the two approaches are not practical. They result in way too much additional work. So most human beings will opt for the only practical route and avoid both of them. Heck many engineers will even avoid testing their own code. After all there is a very real chance they will run into a problem, which would slow down their work and result in poor performance reviews.
If a team works like this for some time, soon the entire code base will deteriorate in to a chaotic mess full of bugs. Worse, it also will result in a frustrated team of engineers not trusting each other’s code, which is a quick way to intoxicate the spirit in every team. People will start doubting each other’s code and blame each other for bugs. Soon good engineers will leave the team and find better opportunities. New engineers have to be hired and trained in all the weird lies and quirks hidden in the code base.
In summary code will be expensive to change, there will be many bugs and people will be very frustrated.
Definition of a Lie
Would the example code above be any better if the method would be just called getNextTask()? At least it would not claim to not update the task? Well, while this is a tiny bit better, it clearly still is not optimal. After all, a reasonable reader would not expect getNextTask() to change the task received.
So for the purpose of this article I define a lie as:
Claiming one thing and then doing something that a reasonable reader would not expect.
A better method name for the above example would be something like getNextTaskAndMarkAsProccessed(). (Yes, the code has other flaws as well – but this is not in the scope of this article.)
Avoiding the Anti-Pattern
Avoiding this anti-pattern is very easy and very straight forward: Simply don’t do it. Make sure your code does exactly what it claims to do and nothing more.
Cleansing a Deteriorated Code Base from the Anti-Pattern
Unfortunately recovering a code base, that contains a few lies, is no trivial task. The only way to reliably ensure, all lies have been removed, would be to carefully read and challenge every single line in the code base. In almost all realistic environments this is simply not an option – it would be way too much work.
So the next best thing is to take an incremental two step approach:
- Make it very clear, that this not acceptable behavior and stop introducing any more lies into the code base.
- Whenever an engineer discovers an instance of a lie he has to resolve it immediately.
With this approach over time the lies in the code base will become fewer and fewer and engineers will slowly gain trust into the code base again. Yes, approach this will initially slow down progress on the code for the foreseeable future, as engineers actually have to test their code again and have to refactor and improve existing parts. After all there is no free lunch. In the long rung this will greatly improve the code base and help to speed up future changes as well as improve the quality of the software significantly.