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
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 -
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
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!