TDD: Listen to the tests… they tell smells in your code!
These days, reading the Goos book, by Steve Freeman and Nat Pryce, it reminded me of a project I worked on a while ago. It was a one year old system, poorly tested, integrating to a handful of other systems, and the code-base... well I prefer not to remember. Despite this scenario, I joined the team to help them implement some new functionalities.
I remember sometimes it was difficult to write tests, the classes were tightly coupled, with no clear responsibilities, several attributes, bloated constructors, etc. And despite our best effort, working around the bits that were preventing us from writing the tests, we felt we were getting down the wrong road, trying to do it in such a crappy code-base. As a result some of our tests were massive! A bunch of lines of mocks, stubs, and expectations, making it impossible to understand their purpose.
What have I learned?
Reading one of the book chapters I learned that the same qualities that makes an object easy to test also makes the code responsive to change. In my situation, the tests were telling me how clumsy the code was and how difficult it would be to extend it.
I also learned that when we come across a functionality that is difficult to test, asking ourselves how to test it is not enough, we also have to ask why is it difficult to test, and check whether it's an opportunity to improve our code. The trick is to do it driven by tests, so we can get rapid feedback on code's internal qualities and on whether it's doing what it's supposed to do.
So they introduced a variation for the well-known TDD cycle— "Write a failing test" => "Make the test pass" => "Refactor". As described on the figure below (extracted from the book), if we're finding it hard to write the next failing test for our application, we should look again at the design of the production code and often refactor it before moving on, until we get to the point that we can write tests that reads well.
Extracted from Growing Object-Oriented Software, Guided By Tests— Steve Freeman and Nat Pryce
An Example of a Smell Tests Might Be Telling You
Reference data rather than behavior
When applying "Tell Don't Ask" or "Law of Demeter" consistently, we end up with a coding style where we tend to pass behavior into the system instead of pulling values up through the stack. So picking up the famous Paperboy example, before refactoring the code applying the "Law of Demeter" the code and test would look something along the lines of the snippet showed below.
class Paperboy
def collect_money(customer, due_amount)
if due_amount > customer.wallet.cash
raise InsuficientFundsError
else
customer.wallet.cash -= due_amount
@total_collected += due_amount
end
end
end
it "should collect money from customer" do customer = Customer.new :wallet => Wallet.new(:amount => 200) paperboy = Paperboy.new paperboy.total_collected.should == 0 paperboy.collect_money(customer, 50) customer.wallet.cash.should == 150 paperboy.total_collected.should == 50 end
We can easily see that the test is telling us it knows too much detail about Customer class implementation. We can see its internals, which objects it's related to, and even worse, we're also exposing implementation details of its peers. So it's clear for me that it needs some design improvement. My main goal here is to hide Customer implementation details from the users of the Paperboy class. Which means that I don't wanna see anything but Customer and Money classes referenced on the test!
class Paperboy
def collect_money(customer, due_amount)
@collected_amount += customer.pay(due_amount)
end
end
it "should collect money from customer" do customer = Customer.new :total_cash => 200 paperboy = Paperboy.new paperboy.total_collected.should == 0 paperboy.collect_money(customer, 50) customer.total_cash.should == 150 paperboy.total_collected.should == 50 end
The method customer.pay(due_amount) wraps all the implementation detail up behind a single call. The client of paperboy no longer needs to know anything about the types in the chain. We've reduced the risk that a design change might cause ripples in remote parts of the codebase.
As well as hiding information, there's a more subtle benefit from "Tell, Don't Ask." It forces us to make explicit and so name the interactions between objects, rather than leaving them implicit in the chain of getters. The shorter version above is much clearer about what it's for, not just how it happens to be implemented.
All the logic necessary to collect the money is inside the Customer object, so it doesn't have to expose its state to its peers.
Now it's safer to continue writing new failing tests to our objects.
Remember, listen to the tests!
Seeking code quality
This week, during another refactoring session, I had the idea that every time I come across code that needs to be modified to conform to one of the principles of best practices for software development, I'll modify it and share the solution through this blog, exploring its drawbacks, benefits gained by conforming to the principle and also providing a brief explanation of it. Starting now with the OCP - Open-Closed Principle.
The Open-Closed Principle
This principle is the foundation for building code that is maintainable and reusable. Modules that conform to this principle are, at the same time, "open for extension", so that they can behave in different ways, as the requirements of the application are modified or as new ones are added. And "closed for modification", which means that the module implementation cannot be violated, nobody can modify its source code.
The description above may seem quite contradictory, since to extend a module functionality, a code modification needs to be done. Therefore, we deduce that a functionality that cannot be modified is given as fixed. How could we deal with this conflict? The best way is to show it up through an example.
In my current project, there's a task manager module, which is responsible for executing asynchronous tasks queued up by its clients. Basically it iterates over all the elements in a pending tasks list and executes each of them, respecting the order they appear. The excerpt below shows how this class was implemented, it's a bit different from the original version, making it easier to understand.
public class TaskManager {
public List pendingTasks;
public void executePendingTasks() {
for (Task task : pendingTasks) {
switch (task.type()) {
case computeTrophies:
engine.computeTrophies();
break;
case computeStatistics:
engine.computeStatistics();
break;
// .... and so on ....
}
}
}
}
The method executePendingTasks() doesn't conform to the Open-Closed Principle, because it cannot be closed against new task implementations. If your client suddenly comes up, asking you to extend the task manager module, to start handling a new task -- this is always happening and since we're all agile, we're ready to handle it eh? -- so this method will always have to be modified to support a new task. This can be easily done by inserting a new case statement, but it's far from an elegant solution.
Unfortunately most of developers follow this approach, the easier one (seemingly), specially when dealing with legacy code, when the solution is already "thought". All they have to do is follow the way the implementation has been done, not assessing it. Eventually, the code ends up with a huge number of lines (eighteen case statements in my case) and at that time, luckily, maybe a sensible developer will realize the code oddity and will try to improve its quality.
In the following excerpt, I'm showing the solution I gave to the problem, conforming to the OCP. Summarizing, the Task class is now abstract and I included an abstract method called execute(). Following this approach, ALL new tasks being created will be derivatives of this abstract class, providing their implementations of the execute() method.
public class abstract Task {
public abstract void execute();
}
public class ComputeTrophiesTask extends Task {
public void execute() {
// logic to compute trophies...
}
}
public class ComputeStatisticsTask extends Task {
public void execute() {
// logic to compute statistics...
}
}
And from now on, to our task manager, executing the pending tasks has became much easier. All it has to do is go through each of them and invoke their execute() method, resulting a cleaner, decoupled and non-intrusive code.
public class TaskManager {
public List pendingTasks;
public void executePendingTasks() {
for (Task task : pendingTasks) {
task.execute();
}
}
}
Now if we want to extend executePendingTasks() to start executing any other new task, all we need to do is create a new derivative of the Task class. This method is now conforming to the Open-Closed Principle. Its behaviour can be extended but its implementation doesn't change.