A kata challenge to try your TDD skills on - failure and what we can learn from it

As promised, today I'd like to show you a way to come up with a suboptimal solution to the kata described earlier.

I'll be using xUnit.NET as my unit test runner and NSubstitute as my mocking library of choice throughout this exercise. In general, I'll be following good practices most of the time and indicate where I'll be "accidentially" losing sight of them.

Specification 1: Creating new instances

We're starting outside-in, from the inputs. In my system, I'll model the most "outside" part of the module as a MessageProcessing class. We should be able to create instances of this class, so the first specification will tell just this:

[Fact]
public void ShouldAllowCreatingNewInstancesWithoutExceptions()
{
  Assert.DoesNotThrow(() => new MessageProcessing());
}

This specification creates a need to have a MessageProcessing class:

public class MessageProcessing
{
}

Ok, so far so good. This, by the way, is a simple example of a Creational Specification. Now let's take on a more complex behavior.

Specification 2: Submitting frame to validation

Ok, so now's the time to use the Given-When-Then analysis. We already have the MessageProcessing class - also, we've got the Frame class. How do they interact with each other?

After giving it a short thought, we can come up with this obvious behavior description:

GIVEN any frame
WHEN message processing is performed for it
THEN it should submit the frame to validation

Cool, let's get to implementing it. The first thing we want to do is to just translate the behavior description above to the code and see which abstractions and methods are missing - then supply them.

The first line:

GIVEN any frame

can be translated to code like this:

var anyFrame = Any.InstanceOf<Frame>();

Then the second line:

WHEN message processing is performed for it

can be written as:

var messageProcessing = new MessageProcessing();
messageProcessing.PerformFor(frame);

Great! Now the last line:

THEN it should submit the frame to validation

and its translation into the code (note the Received() extension method, which is an NSubstitute construct):

validation.Received().PerformFor(anyFrame);

Ok, so we've made the translation, now let's summarize this and see what's missing to make this code compile:

[Fact] public void 
ShouldSubmitFrameToValidationWhenPerformedForThatFrame()
{
  //GIVEN
  var anyFrame = Any.InstanceOf<Frame>();

  //WHEN
  var messageProcessing = new MessageProcessing();
  messageProcessing.PerformFor(frame);

  //THEN
  validation.Received().PerformFor(anyFrame);
}

What's missing? We have to somehow create the validation and pass it to MessageProcessing, since it's the entity responsible for performing the validation. And so, the corrected version looks like this.

[Fact] public void 
ShouldSubmitFrameToValidationWhenPerformedForThatFrame()
{
  //GIVEN
  var anyFrame = Any.InstanceOf<Frame>();
  var validation = Substitute.For<Validation>();  

  //WHEN
  var messageProcessing = new MessageProcessing(validation);
  messageProcessing.PerformFor(frame);

  //THEN
  validation.Received().PerformFor(anyFrame);
}

As you can see, we've discovered one new abstraction - a validation, and two new methods: one for message processing and one for validation (accidentially, both named PerformFor()). Thus, we have created a need for all this new stuff to exist. Additionally, we made a mistake when dividing responsibilities between classes, and we'll see why later.

By the way, this is what's called a Work-Flow Specification. Our workflow is very primitive, since it consists of forwarding a method call to another object. In real-world scenarios (and in the correct solution to this kata) workflows are more worthwile.

After creating all the necessary types and methods, the implementation looks like this:

public class MessageProcessing
{
  public MessageProcessing(Validation validation)
  {
    throw new NotImplementedException();
  }

  public void PerformFor(Frame frame)
  {
    throw new NotImplementedException();
  }
}

//TODO implement
public interface Validation
{
  void PerformFor(Frame frame); 
}

Also, we've got to correct the first specification to cater for new construction argument:

[Fact]
public void ShouldAllowCreatingNewInstancesWithoutExceptions()
{
  Assert.DoesNotThrow(
    () => new MessageProcessing(Any.InstanceOf<Validation>())
  );
}

Note that I added a TODO next to the Validation interface - this is because I'll need to go back later and actually implement it. While I don't write much about a TODO list in this post, it's actually one of TDDer's best friends (maybe I'll write a post about it someday).

Let's leave the validation for now. To make this specification I just wrote pass, I'll need to change the MessageProcessing class:

public class MessageProcessing
{
  private readonly Validation _validation;

  public MessageProcessing(Validation validation)
  {
    _validation = validation;
  }

  public void PerformFor(Frame frame)
  {
    _validation.PerformFor(frame);
  }
}

Now it's all green, so let's examine our TODO list. The only item on the list is the Validation interface implementation, so let's go ahead and create...

Specification 3: new instances of concrete validation.

So we need a concrete implementation of the Validation interface. Let's call it BasicValidation, since the logic in there is gonna be really simple.

[Fact]
public void ShouldAllowCreatingNewInstancesWithoutExceptions()
{
  Assert.DoesNotThrow(
    () => new SanityValidation()
  );
}

and now we've created a need for a class named SanityValidation. The code for SanityValidation class looks like this now:

public class SanityValidation : Validation
{
  public void PerformFor(Frame frame)
  {
    throw new NotImplementedException();
  }
}

Some tools, like Resharper, put occurences of NotImplementedException on the TODO list, which is a great feature, because replacing it with concrete implementation actually IS something "to do".

Specification 4: Ignoring valid frames

Starting from this chapter, we're going to feel the pain of my bad design decision more and more. In the grand finale, I'll show you what mess we've got ourselves into. But first things first, let's write the first specification of validation, which says:

GIVEN a validation and a frame
AND the frame has all its fields valid
WHEN the frame is submitted to validation
THEN no exception should be thrown

which, translated into code, looks like this:

[Fact] public void 
ShouldNotThrowAnyExceptionWhenPerformedForFrameWithAllFieldsValid()
{
  //GIVEN
  var frame = new Frame()
  { 
    Speed = SanityValidation.MinValidInteger,
    Age = SanityValidation.MinValidInteger,
    Sender = Any.NonNullNonEmptyString()
  };

  var validation = new SanityValidation();

  //WHEN - THEN
  Assert.DoesNotThrow(
    () => validation.PerformFor(frame)
  );
}

(By the way, this is called a Functional Specification.)

Interesting... we had to know exactly what "all fields" mean. I wonder what will be the effect of adding new field to the frame in the future... We'll see more of that with upcoming specs. For now, let's just type in the implementation which is to remove throwing the NotImplementedException() from PerformFor() method. Oh, I almost forgot, we introduced a need to create a constant named MinValidInteger, so we have to add it to the SanityValidation class:

public class SanityValidation : Validation
{
  public const MinValidInteger = 12345; //TODO
  public void PerformFor(Frame frame)
  {
  }
}

Note that we're not assigning this new constant any particular value - 12345 is just a dummy number. Moreover, we're putting a TODO next to the constant, since we'll want to revisit it later and actually write a specification on what the value of the constant should be. But for now - done. Time for the next one.

Specification 5: throwing exception on invalid Speed

Let's describe the expected behavior:

GIVEN a validation and a frame
AND the frame has all its fields valid except for the Speed
WHEN the frame is submitted to validation
THEN an exception should be thrown

Note the "all fields" again... this is becoming very interesting. let's see what comes up when we put the spec into code:

[Fact] public void 
ShouldThrowAnExceptionWhenPerformedForFrameWithInvalidSpeed()
{
  //GIVEN
  var frame = new Frame()
  { 
    Speed = SanityValidation.MinValidInteger - 1,
    Age = SanityValidation.MinValidInteger,
    Sender = Any.NonNullNonEmptyString()
  };

  var validation = new SanityValidation();

  //WHEN - THEN
  Assert.Throws<Exception>(
    () => validation.PerformFor(frame)
  );
}

Another Functional Specification. Note that we're repeating assignments to all of the frame fields. Also, we're repeating the knowledge on what it means to be "valid" for each field except the speed. Keep calm, the grand finale is getting nearer and nearer. For now, let's just pretend that nothing happened and add the necessary code to make this spec pass:

public class SanityValidation : Validation
{
  public const MinValidInteger = 12345; //TODO
  public void PerformFor(Frame frame)
  {
    if(frame.Speed < MinValidInteger)
      throw new Exception();
  }
}

Specification 6: throwing exceptions on invalid Age

To be honest, nothing really interesting happens here, since it's almost the same as in the previous case. Let's just write the spec:

[Fact] public void 
ShouldThrowAnExceptionWhenPerformedForFrameWithInvalidAge()
{
  //GIVEN
  var frame = new Frame()
  { 
    Speed = SanityValidation.MinValidInteger,
    Age = SanityValidation.MinValidInteger - 1,
    Sender = Any.NonNullNonEmptyString()
  };

  var validation = new SanityValidation();

  //WHEN - THEN
  Assert.Throws<Exception>(
    () => validation.PerformFor(frame)
  );
}

and the code necessary to make it pass:

public class SanityValidation : Validation
{
  public const MinValidInteger = 12345; //TODO
  public void PerformFor(Frame frame)
  {
    if(frame.Speed < MinValidInteger)
      throw new Exception();
    if(frame.Age < MinValidInteger)
      throw new Exception();
  }
}

By the way, is it only me, or is there a pattern recurring in both specs and production code? Oh, and the conclusions on bad design drawn during the previous specification are only reinforced here!

Specification 7: Sender cannot be null

Also, this one is analogous to the two previous ones, only this time we'll be checking for null instead of a numeric boundary:

[Fact] public void 
ShouldThrowAnExceptionWhenPerformedForFrameWithNullSender()
{
  //GIVEN
  var frame = new Frame()
  { 
    Speed = SanityValidation.MinValidInteger,
    Age = SanityValidation.MinValidInteger,
    Sender = null
  };

  var validation = new SanityValidation();

  //WHEN - THEN
  Assert.Throws<Exception>(
    () => validation.PerformFor(frame)
  );
}

and here's the code:

public class SanityValidation : Validation
{
  public const MinValidInteger = 12345; //TODO
  public void PerformFor(Frame frame)
  {
    if(frame.Speed < MinValidInteger)
      throw new Exception();
    if(frame.Age < MinValidInteger)
      throw new Exception();
    if(frame.Sender == null)
      throw new Exception();
  }
}

...and the last validation:

Specification 8: Sender cannot be empty

Almost exactly the same as the previous one. Comments are not needed. Here's the spec:

[Fact] public void 
ShouldThrowAnExceptionWhenPerformedForFrameWithEmptySender()
{
  //GIVEN
  var frame = new Frame()
  { 
    Speed = SanityValidation.MinValidInteger,
    Age = SanityValidation.MinValidInteger,
    Sender = string.Empty
  };

  var validation = new SanityValidation();

  //WHEN - THEN
  Assert.Throws<Exception>(
    () => validation.PerformFor(frame)
  );
}

and production code that fulfills it:

public class SanityValidation : Validation
{
  public const MinValidInteger = 12345; //TODO
  public void PerformFor(Frame frame)
  {
    if(frame.Speed < MinValidInteger)
      throw new Exception();
    if(frame.Age < MinValidInteger)
      throw new Exception();
    if(string.IsNullOrEmpty(frame.Sender))
      throw new Exception();
  }
}

and that's it for the validation logic - we've got all the behaviors nailed. The only thing left to do before I explain where (and why) I failed, is a specification for the constant that pollutes our TODO list:

Specification 9: Minimum Valid Integer for validations should be 1

The spec is really simple:

[Fact] public void 
ShouldUse1AsMinimumValidInteger()
{
  Assert.Equal(1, SanityValidation.MinValidInteger);
}

This thing is called a Constant Specification. To make it pass (because now it's failing - remember that we put 12345 as the value for MinValidInteger), we'll just have to change the value of the constant to 1:

public class SanityValidation : Validation
{
  public const MinValidInteger = 1;

  public void PerformFor(Frame frame)
  {
    if(frame.Speed < MinValidInteger)
      throw new Exception();
    if(frame.Age < MinValidInteger)
      throw new Exception();
    if(string.IsNullOrEmpty(frame.Sender))
      throw new Exception();
  }
}

Ok, all behaviors nailed down, all specifications written, now it's time for a short retrospective.

Where (and why) did I go wrong?

The first mistake I made was in Specification 2. I just passed the whole frame to validation. What's wrong with that? By doing this, I failed to encapsulate the knowledge about the structure of the frame and eventually led to coupling all objects of the application to the Frame type. The frame is a third party class (you can't control how it changes), plus it's a data class (and data classes are code smells) - never couple your application to something as horrible as this!. Imagine the next version of the module would introduce compression and we'd handle this by introducing another class called Compression, then pass whole frame to it. Now every change to the Frame would impact both Validation and Compression object in the same way, introducing redundancy in dependencies and a maintainability pain - stronger with each new class that would need to operate on the frame. One of the goals of object oriented design is to keep data together with behaviors related to that data and that's exactly what I failed to achieve.

The other mistake was to further fail to encapsulate the frame structure when it got into the Validation object. If you look at specifications 4, 5, 6, 7 and 8 - all of them contain both the knowledge on which fields to validate and what "valid" means for each field. This led to many repetitions in the specs (adding a new field to Frame class would require changes in five specs plus adding a new one - a real horror!) and exposed smells in the production code itself, which are: violation of the Single Responsibility Principle (again, Validation is responsible for deciding which fields to validate as well as what the validations mean, making it bear two responsibilities) and redundancy (note that Age and Speed are validated the same way - the specification in my previous post did not assign a validation rule to fields, but to types - and I failed to eliminate the duplication between validations of two fields that are of the same type). And this is only for three fields! To give you a feel of the maintenance horror I mentioned, let's imagine how the truth table would look like if we had ten fields:

	Spec 1	Spec 2	Spec 3	Spec 4	Spec 5	Spec 6	Spec 7	Spec 8	Spec 9	Spec 10	Spec 11
Field 1	V	X	V	V	V	V	V	V	V	V	V
Field 2	V	V	X	V	V	V	V	V	V	V	V
Field 3	V	V	V	X	V	V	V	V	V	V	V
Field 4	V	V	V	V	X	V	V	V	V	V	V
Field 5	V	V	V	V	V	X	V	V	V	V	V
Field 6	V	V	V	V	V	V	X	V	V	V	V
Field 7	V	V	V	V	V	V	V	X	V	V	V
Field 8	V	V	V	V	V	V	V	V	X	V	V
Field 9	V	V	V	V	V	V	V	V	V	X	V
Field 10	V	V	V	V	V	V	V	V	V	V	X

You get the issue, right? This code should be refactored, but I'm not going to do this, because I want to show you the correct solution next time. In the meantime, you can try to refactor yourself and see what you end up with. As always, comments are welcome.

But until then... see ya!