Many thanks to Łukasz Bigos for discussion and inspiration.
Recently I had a discussion with few of my colleagues on readability of Test-driven (or more precisely - loosely coupled) code. The thing that surprised me the most was the argument that loosely coupled code is unreadable. The surprise comes from my deep conviction that TDD help achieve extremely readable code.
In my mind, TDD leads to well factored, small classes with single responsibilities, but in some others' minds, it's just "obfuscating the picture" and they "don't know what's happening in this code because everything is hidden and split across many classes and methods".
So I thought about it some more, talked a little more and finally I reached a conclusion on why there's a disagreement between us.
Two ways of understanding readability
It appears that, depending on our habits, "readable" means something different to us. There are two kinds of readability I learned to differentiate: implementation readability and design readability. Let's try to look at what each one is about and who & why finds this particular kind of readability relevant.
1. Implementation readability
Let's imagine we're developing client-server application for project management. The part we'll be dealing with is the server-side processing of a request to add new issue to existing project. By looking at the example code, we'll see how and why implementation readability plays out in practice. Without further ado, ladies and gentlemen, the code:
public void AddNewIssueToProject(Request request, Response response)
{
try
{
if(!request.ContainsKey("senderIp")
|| string.IsNullOrWhiteSpace(request["senderIp"] as string))
{
response.Write("Wrong sender IP");
return;
}
if(!request.ContainsKey("projectId")
|| string.IsNullOrWhiteSpace(request["projectId"] as string))
{
response.Write("Invalid project ID");
return;
}
if(!request.ContainsKey("issueData") ||
request["issueData"] as Dictionary<string, string> == null)
{
response.Write("issue data not passed");
return;
}
var issueData
= request["issueData"] as Dictionary<string, string>;
if(!issueData.ContainsKey("Title") ||
string.IsNullOrWhiteSpace(issueData["Title"]))
{
response.Write("No title for issue supplied. " +
"Issue title must be at least one readable character");
return;
}
if(!issueData.ContainsKey("Content") ||
string.IsNullOrWhiteSpace(issueData["Content"]))
{
response.Write("No content for issue supplied. " +
"By our policy, every issue must be described with details");
return;
}
if(!issueData.ContainsKey("Severity")
|| string.IsNullOrWhiteSpace(issueData["Severity"]))
{
response.Write("No severity supplied, " +
"although the form forces this." +
" Are you trying to send us raw HTTP request?");
return;
}
var projectId = request["projectId"] as string;
var issueTitle = issueData["Title"];
var issueContent = issueData["Content"];
var severity = issueData["Severity"];
var queryString = string.Format(
"INSERT INTO ProjectIssues VALUES ({0}, '{1}', '{2}', '{3}')",
projectId, issueTitle, issueContent, severity);
var query = new Query(queryString);
using(var connection = DatabaseConnection.Open("dbName=Abcdef"))
{
query.ExecuteThrough(connection);
}
response.Write("Everything's OK!");
}
catch(Exception e)
{
response.Write("Unexpected error, see log for details");
_log.Error(e);
}
}
While this snippet may seem like a classic spaghetti code to those of you who are more design-infected, there is actually a significant benefit behind it. The benefit is that without looking anywhere else, we can deduce the state of the method variables in every line and what precisely will happen in the system when we run this code. Because the method is made up mostly of primitive or library constructs, we're able to "debug" it with our eyes only, without jumping here and there to gather the parts of the functionality necessary to "understand" the code.
Wait, did I just write "understand"? What kind of "understanding" are we talking about here? Well let's take
the following line as example:
var projectId = request["projectId"] as string;
When reading this code, by the time we arrive at this line we already know that some values are put inside the dictionary and that they're non-null and not some other value like empty string. What are "dictionary", "null" and "empty string"? They're implementation details! Are they connected to the domain of request processing? No. Do they help describe the high-level work-flow? No. Do they help us understand the requirements? Not much. Can we read from the code what steps does this method consists of or where each step begins and where it ends? No, we can extract it somehow by comparing the code to our domain knowledge or experience (e.g. most of us know that each request coming to the server has to be validated), but again, this is something extracted from the code, not something that's already there.
So, the conclusion is that this style of writing is better at describing how the code works, while doing a poor job of describing what the code is for.
Benefits of implementation readability
So, who benefits from having a high implementation readability? Well, there are times when we're given some assignments in a code we don't know. Let's say that our boss told us to fix a bug in an application that should write string "abcd" to Windows registry, but it writes "abc". Now, we don't care about the big picture - all we care for is that we know what the single external behavior is and what it should be instead (we don't even care why), so we search for this one line in the code that is responsible for the behavior to replace it with something else. While searching, everything that is not the sought construct (including design and domain knowledge), is in our way. From this point of view, the ideal situation would be to have the whole application in a single source file so that we can use our text editor to search it for the word "Registry", then examine each occurrence and make the fix. In other words, we're acting as a little smarter "search and replace" mechanisms. The single-responsibility classes just get in our way and make us "pointlessly" navigate through a web of objects we neither want nor need to understand (some would say that we're on level 1 or 2 of Dreyfus model of skill acquisition).
While cases such as this one happen, we must realize that they're not the cases we're trying to optimize for. In any but the simplest cases, making a novice go into a piece of code to make changes without understanding of domain or design will lead to degradation of the code and probably few bugs (when someone is not aware of the big picture, they might fix a bug in a way that introduces another bug somewhere else, because of another scenario they didn't have any idea of). So the case we want to optimize for is someone going into the code with a need to understand the domain and the design first before making a change. It's best if they can derive at least part of this knowledge from the code itself and easily map parts of domain knowledge to places in the code. And this is how we arrive at...
2. Design readability
Part of good object-oriented design is implementation hiding, i.e. we want to hide how a particular piece of functionality is implemented so that we can change it later. Also, when we're equipped with domain and design knowledge, we want this knowledge stand out, not to be obfuscated by implementation details. To give you a quick example of what I mean: when talking about web server session storage, we say that "a user is assigned a persistent session", not that "a database holds a serialized hashtable indexed by user ID". Thus, we want to see the first clearly visible in the code, not the latter. Otherwise, readability is hurt.
Let's now take a little piece of refactored request handling code that we began with and see whether we can see any improvements:
class AddingNewIssueToProject : Scenario
{
UserInteraction _user;
AddIssueToProjectRequest _addIssueToProjectRequest;
PersistentStorage _issueStorage;
public AddingNewIssueToProject (
AddIssueToProjectRequest requestForAddingIssueToProject,
UserInteraction userInteraction,
PersistentStorage issueStorage)
{
this._requestForAddingIssueToProject
= requestForAddingIssueToProject;
this._user = userInteraction;
this._issueStorage = issueStorage;
}
public void GoThrough()
{
try
{
_requestForAddingIssueToProject.Validate();
Issue issue = _requestForAddingIssueToProject.ExtractIssue();
issue.StoreInside(_issueStorage);
issue.ReportSuccessfulAdditionTo(_user);
}
catch(ScenarioFailedException scenarioFailed)
{
_user.NotifyThat(scenarioFailed);
}
}
}
As you can see, there's almost no "implementation" here. No strings, no nulls, no dictionaries, lists, data structures, no addition, subtraction, multiplication etc. So how can it be readable as it tells very little of the external application behavior? Count with me:
- The class name is AddingNewIssueToProject, which says that this class is all about this single scenario (in other words, when something is wrong with different scenario, this is not the place to look for root cause). Moreover, it implements a Scenario interface, which gives us a clue that each scenario in the application is modeled as object and we can most probably find all scenarios supported by the application by searching for classes implementing Scenario interface.
- It depends on three interfaces: UserInteraction, AddIssueToProjectRequest and PersistentStorage, which suggest that the scope of the scenario is: communication with user, working with the user-entered data and saving data to persistent storage.
- The GoThrough() method contains the sequence of steps. We can clearly see which steps the scenario consists of and what is their order, e.g. looking at this method, we are sure that we're not trying to save invalid Issue object, since storing issue is after validating correctness.
- Looking at the invocation of ExtractIssue() method, we can see that the new issue that's being saved is made of data entered by user.
- The object _issueStorage is of type PersistentStorage, suggesting that, after the issue is saved, we'll be able to retrieve it later, most probably even after the server restart.
- Each of the steps of the scenario is allowed to fail (by throwing ScenarioFailedException) and the user is immediately notified of the failure.
- If the adding issue scenario ever needs to perform any additional step in the future, this is the place where we'd start adding this change.
Not bad for just few lines of code that contain practically no implementation details, huh?
Which one to prefer?
Personally, I strongly prefer design readability, i.e. I want to draw as much domain knowledge and design constraints knowledge from the code (not to be mistaken with overdesign) as possible. One reason for this is that design readability makes it much easier to introduce changes that are aligned with the "spirit" of the code that's already in place and such changes are usually much more safe than hacking around. Another reason is that implementation readability is impossible to maintain over the long haul. Let's take the following snippet from our example:
if(!issueData.ContainsKey("Severity")
|| string.IsNullOrWhiteSpace(issueData["Severity"]))
{
response.Write("No severity supplied, " +
"although the form forces this." +
" Are you trying to send us raw HTTP request?");
return;
}
This is a part of code responsible for adding new issue, but we can imagine it making its way to scenario for editing existing issue as well. When this happens, we have two choices:
-
Copy-paste the code into the second method, creating duplication and redundancy (and leading to situations where one change needs to be made in more than one place, which, according to Shalloway's Law is asking for trouble)
-
Extracting the code at least to a separate method that already defeats implementation readability partially, because each time you need to understand what piece of code does, you need to understand an additional method. All you can do is to give this method an intent-revealing name. Anyway, introducing new methods just to remove redundancy leaves us with with code where sections of implementation details are mixed with sections of domain work-flows stated in terms of calling domain-specific methods on domain-specific objects. From such code, we can easily deduce neither full work-flows, nor all implementation details. Such code is better known under the name of Spaghetti Code :-),
An object oriented system is not a set of streams of primitive instructions. It is a web of collaborating objects. The more complex the system, the more this metaphor becomes visible. That's why, instead of trying to put all implementation in one place to make it "debuggable", it's better to create a cohesive and readable description of that collaboration. A description that reveals domain rules, domain workflows and design constraints.
Of course, the choice is yours. Until you decide to do TDD, that is. That's because TDD strongly encourages clean design and separation of domain and design from implementation details. If good design guidelines are not followed, tests get longer and longer, slower and slower, more and more test logic is repeated throughout the tests etc. If that's where you are now, better revise your approach to design!
What do you think?
I'm dying to know what your opinions and observations on these topics are. Do you find the distinction between implementation and design readability useful? Which one you prefer? Please let me know in the comments!
