Implementation Readability vs Design Readability and what TDD has to do with it?

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:

1. 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.
2. 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.
3. 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.
4. 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.
5. 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.
6. Each of the steps of the scenario is allowed to fail (by throwing ScenarioFailedException) and the user is immediately notified of the failure.
7. 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:

1. 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)
2. 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!

The Two Main Techniques in Test-Driven development, part 2

Today, I'd like to continue where I left last time. As you might remember, the first post was about Need-Driven Development and this one's gonna be about a technique that's actually older and it's called Triangulation.

Triangulation

History

The first occurence of the term triangulation I know about is in Kent Beck's book Test Driven Development: By Example where Kent describes it as the most conservative technique of test-driving the implementation. It's one of the three core techniques of classic TDD.

Description

The three approaches to test-driving the implementation and design described in Kent's book are:

1. Write the obvious implementation
2. Fake it ('till you make it)
3. Triangulate

Kent describes triangulation as the most conservative technique, because following it involves the tiniest possible steps to arrive at the right solution. The technique is called triangulation by analogy to radar triangulation where outputs from at least two radars must by used to determine the position of a unit. Also, in radar triangulation, the position is measured indirectly, by combining the following data: range (not position!) measurement done by the radar and the radar's own position (which we know, because we put the radar there).

These two characteristics: indirect measurement and using at least two sources of information are at the core of TDD triangulation. Basically, it says:

1. Indirect measurement: Derive the design from few known examples of its desired external behavior by looking at what varies in these examples and making this variability into something more general
2. Using at least two sources of information: start with the simplest possible implementation and make it more general only when you have two or more examples

Triangulation is so characteristic to the classic TDD that many novices mistakenly believe TDD is all about triangulation.

Example

Suppose we want to write a method that creates an aggregate sum of the list. Let's assume that we have no idea how to design the internals of our custom list class so that it fulfills its responsibility. Thus, we start with the simplest example:

[Test]
public void 
ShouldReturnTheSameElementAsASumOfSingleElement()
{
  //GIVEN
  var singleElement = Any.Integer();
  var listWithAggregateOperations 
    = new ListWithAggregateOperations(singleElement);

  //WHEN
  var result = listWithAggregateOperations.SumOfElements();

  //THEN
  Assert.AreEqual(singleElement, result);
}

The naive implementation can be as follows:

public class ListWithAggregateOperations
{
  int _element;

  public ListWithAggregateOperations(int element)
  {
    _element = element;
  }

  public int SumOfElements()
  {
    return _element;
  }
}

It's too early to generalize yet, as we don't have another example. Let's add it then. What would be the next more complex one? Two elements instead of one. As we can see, we're beginning to discover what's variable in our design. The second spec looks like this:

[Test]
public void 
ShouldReturnSumOfTwoElementsAsASumWhenTwoElementsAreSupplied()
{
  //GIVEN
  var firstElement = Any.Integer();
  var secondElement = Any.Integer();
  var listWithAggregateOperations 
    = new ListWithAggregateOperations(firstElement, secondElement);

  //WHEN
  var result = listWithAggregateOperations.SumOfElements();

  //THEN
  Assert.AreEqual(firstElement + secondElement, result);
}

And the naive implementation will look like this:

public class ListWithAggregateOperations
{
  int _element1 = 0;
  int _element2 = 0;

  public ListWithAggregateOperations(int element)
  {
    _element1 = element;
  }

  //added
  public ListWithAggregateOperations(int element1, int element2)
  {
    _element1 = element1;
    _element2 = element2;
  }

  public int SumOfElements()
  {
    return _element1 + _element2; //changed
  }
}

Now the variability in the design becomes obvious - it's the number of elements added! So now that we have two examples, we see that we have redundant constructors and redundant fields for each element in the list and if we added a third spec for three elements, we'd have to add another constructor, another field and another element of the sum computation. Time to generalize!

How do we encapsulate the variability of the element count so that we can get rid of this redundancy? A collection! How do we generalize the addition of multiple elements? A foreach loop through the collection!

First, let's start with writing a new, more general unit spec to showcase the new desired design (the existing two specs will remain in the code for now):

[Test]
public void 
ShouldReturnSumOfAllItsElementsWhenAskedForAggregateSum()
{
  //GIVEN
  var firstElement = Any.Integer();
  var secondElement = Any.Integer();
  var thirdElement = Any.Integer();
  var listWithAggregateOperations 
    = new ListWithAggregateOperations(new List<int>
      { firstElement, secondElement, thirdElement});

  //WHEN
  var result = listWithAggregateOperations.SumOfElements();

  //THEN
  Assert.AreEqual(firstElement + secondElement + thirdElement, result);
}

Note that we have introduced a constructor taking a list of arbitrary number of elements instead of just the values. Time to accommodate it in the design and bring the generalization we have just introduced in our spec into the implementation:

public class ListWithAggregateOperations
{
  List<int> _elements = new List<int>();

  public ListWithAggregateOperations(int element)
    : this(new List<int>() { element }) //changed
  {
  }

  public ListWithAggregateOperations(int element1, int element2)
    : this(new List<int>() { element1, element2 }) //changed
  {
  }

  //added
  public ListWithAggregateOperations(List<int> elements)
  {
    _elements = elements;
  }

  public int SumOfElements()
  {
    //changed
    int sum = 0;
    foreach(var element in _elements)
    {
      sum += element;
    }
    return sum;
  }
}

As we can see, the design is more general and I made the two existing constructors as a simple delegation to a new, more general one. Also, the first two specs ("one element" and "two elements") still pass with the new, more general implementation under the hood, meaning that we didn't break any existing behavior. Thus, it's now safe to remove those two specs, leaving only the most general one. Also, we can remove the redundant constructors, leaving the implementation like this:

public class ListWithAggregateOperations
{
  List<int> _elements = new List<int>();

  public ListWithAggregateOperations(List<int> elements)
  {
    _elements = elements;
  }

  public int SumOfElements()
  {
    int sum = 0;
    foreach(var element in _elements)
    {
      sum += element;
    }
    return sum;
  }
}

And voilà! We have arrived at the final, generic solution. Note that the steps we took were tiny - so you might get the impression that the effort was not worth it. Indeed, this example was only to show the mechanics of triangulation - in real life, if we encountered such simple situation we'd know straight away what the design would be and we'd start with the general specs straight away and just type in the obvious implementation. Triangulation shows its power in more complex problems with multiple design axes and where taking tiny steps helps avoid "analysis paralysis".

Principles

These are the main principles of Triangulation:

1. Start writing specs from the simplest and most obvious case, increasing the complexity of the specs and the generality of implemntation as you add more specs.
2. Generalize only when you have at least two examples that show you which axis of design change needs to be generalized.
3. After arriving at the correct design, remove the redundant specs (remember, we want to have one spec failing for single reason)

Related Concepts

Inside-out development

Triangulation is a core part of inside-out TDD style, where one uses mocks sparingly and focuses on getting the lower-layer (i.e. the assumptions) right before developing a more concrete solution on top of it.

Can triangulation be used as a part of outside-in development (as described last time)? Of course, although it's probably used less often. Still, when you have a piece of functionality with well-defined inputs and outputs but don't know what the design behind it could be, you can go ahead and use triangulation whether you're developing outside-in or inside-out.

Acceptance tests/specifications

Even when doing Need-Driven Development using mocks etc., triangulation can be very useful at the acceptance level, where you can try to derive the internal design of whole module based on the tests that convey your understanding of domain rules.

Applicability

As I stated before, triangulation is most useful when you have no idea how the internal design of a piece of functionality will look like (e.g. even if there are work-flows, they cannot be easily derived from your knowledge of the domain) and it's not obvious along which axes your design must provide generality, but you are able to give some examples of the observable behavior of that functionality given certain inputs. These are usually situations where you need to slow down and take tiny steps that slowly bring you closer to the right design - and that's what triangulation is for!

Personally, I find triangulation useful when test-driving non-trivial data structures.