Good Practices when Reviewing a Domain Model: 10 Years of .Net Compressed into Weeks #9
on
This post is part of a blog series ASP.Net 10 Years On. Even though this is part of a series I have tried to make each post standalone.
For the time being we have completed the domain model so now is a good time to take a few steps back and review it. It's much improved from where we started, but during this process some issues have crept in.
This post covers:
- Do you have enough Unit Test Coverage?
- Are you suffering from Feature Envy?
- Is your Code Too Expensive?
- Remember: Tell, Don't Ask!
- Scrutinise your Class Dependencies: new = Glue
- Why it's a good idea to use an EntityBase
- Are your unit tests crossing boundaries?
- Use Extension Methods to contain Generic Functions
- Unit Tests to the Rescue
Do you have enough Unit Test Coverage?
The first step is to ensure the unit test coverage is good. This maximises the chances of catching any bugs that may be introduced if any changes are made as a result of this code review.
Products such as NCover offer code coverage reporting and some versions of Visual Studio have code coverage tools included. I'm using Visual Studio Ultimate (which does) and have configured VS to run tests and view code coverage.
I ran this on the domain model and it reported an average of 84.57% coverage. Not bad! Here is the breakdown:
I wouldn't try to change the code to make this figure 100% as constructors are counted in this figure. Sometimes it is sensible to be pragmatic. A tool such as this is no substitute for developer skill, rather it's a "nice to have" in the tool box.
A nice feature is that it highlights covered code so that you can see visually what has been covered by a test. The code highlighted in grey is covered by tests, so I can see this method has 100% coverage:
Tools such as this are handy, but I don't like to use them as I go as I find it interrupts my flow when writing code. I use the following approach:
A simpler way of determining code coverage as you write tests is to count the IF, And, Or, Case, For and While statements.
Take a look at the following method and count how many tests we should have to cover this adequately?
public override void PickWinner(Competition competition)
{
if (competition.ClosingDateHasPassed)
{
Entrant winningEntrant = null;
if (competition.Statistics.HasCorrectAnswers)
{
winningEntrant = WinnerSelector.GetWinner(competition.Statistics)));
}
DomainEvents.Raise(new WinnerSelectedEvent(competition, winningEntrant));
competition.SetCompetitionState(new ClosedState());
}
}
I make it 3 tests:
- All conditions as true: We test that the event is raised with a valid winner and the state is set to closed.
- Everything in a false state: We test the event is not raised on the state is not changed.
- If the first IF is true and the second false: It means the date has passed but there are no correct answers, so test that the event is raised but with a null winner.
This is touching on the concept of Cyclomatic complexity where we are trying to ensure that the independent paths our code may take is covered by a test.
Are you suffering from Feature Envy?
What's wrong with this method? This simple method was taken from the OpenState class that accepts the Competition class via its constructor:
private static bool ClosingDateHasPassed(Competition competition)
{
return (competition.ClosingDate > DateTime.Now);
}
I will give you a hint taken from Refactoring: Improving the Design of Existing Code:
A class smell is a method that seems more interested in a class other than the one it is actually in.
This is a very simple example, but this logic should live in the Competition class:
Before
public override void PickWinner(Competition competition)
{
if (ClosingDateHasPassed(competition))
{
// some logic goes here…
}
}
private static bool ClosingDateHasPassed(Competition competition)
{
return (competition.ClosingDate > DateTime.Now);
}
After
public override void PickWinner(Competition competition)
{
if (competition.ClosingDateHasPassed)
{
// some logic goes here…
}
}
Is your Code Too Expensive?
Refactoring: Improving the Design of Existing Code postulates that:
Each class you create costs money to maintain and understand.
I had a class in the domain model that didn't feel right. I wasn't sure about the name and for some reason it bothered me. Here is the bothersome class:
public class WinnerSelector
{
public static CompetitionWinner GetWinner(CompetitionStatistics statistics)
{
CompetitionWinner winner = new CompetitionWinner();
winner.Entrant = statistics.CorrectAnswers.ElementAt(GetRandomEntrantIndex());
return winner;
}
/// Gets the random index of the entrant.
private int GetRandomEntrantIndex(CompetitionStatistics statistics)
{
return new Random().Next(statistics.CorrectAnswers.Count());
}
}
It has a touch of feature envy since it's asking a lot of questions about the competition statistics class. So maybe we should move those two methods into the CompetitionStatistics class? The code that calls this class has now changed:
Before
public override void PickWinner(Competition competition)
{
if (competition.ClosingDateHasPassed)
{
Entrant winningEntrant = null;
if (competition.Statistics.HasCorrectAnswers)
{
// *** Changing *** //
winningEntrant = WinnerSelector.GetWinner(competition.Statistics)));
}
DomainEvents.Raise(new WinnerSelectedEvent(competition, winningEntrant));
competition.SetCompetitionState(new ClosedState());
}
}
After
public override void PickWinner(Competition competition)
{
if (competition.ClosingDateHasPassed)
{
Entrant winningEntrant = null;
if (competition.Statistics.HasCorrectAnswers)
{
// *** Changed *** //
winningEntrant = competition.Statistics.GetWinnerAtRandom();
}
DomainEvents.Raise(new WinnerSelectedEvent(competition, winningEntrant));
competition.SetCompetitionState(new ClosedState());
}
}
The fact that we moved these methods out of the WinnerSelector class means we can now delete this class and our code base just got a little cheaper!
Remember: Tell, Don't Ask!
Did anybody spot a problem in the previous code example? I'll offer a hint by highlighting the offending block of code:
if (competition.Statistics.HasCorrectAnswers)
{
winningEntrant = competition.Statistics.PickWinnerAtRandom();
}
Did you spot it? The problem is that we are asking the CompetitionStatistics class a question about its state before we call a method on the same class.
Tell, Don't Ask states:
Procedural code gets information then makes decisions. Object-oriented code tells objects to do things.
This means that the logic to check if there are correct answers should be situated within the PickWinnerAtRandom() method.
After reafactoring we have one line of code:
winningEntrant = competition.Statistics.PickWinnerAtRandom();
The logic has been moved into the method on the CompetitionStatistics class:
public Entrant PickWinnerAtRandom()
{
if (HasCorrectAnswers)
{
return CorrectAnswers.ElementAt(GetRandomEntrantIndex());
};
return null;
}
Scrutinise your Class Dependencies: new = Glue
Reviewing the classes for dependencies revealed the following constructor:
public Competition(IEnumerable<Entrant> entrants, ICompetitionState state)
{
CreationDate = DateTime.Now;
PossibleAnswers = new PossibleAnswers();
State = state;
Statistics = new CompetitionStatistics(this, entrants);
}
What's the issue here? Here's a hint:
When trying to hunt down unwanted class dependencies, keep in mind that new = Glue.
The thing that grabbed my attention was the following line:
Statistics = new CompetitionStatistics(this, entrants);
This line of code is tightly coupling this class to the CompetitionStatstics class. This kind of makes sense since they need each other to function. The only reason for the creation of the class is to reduce the size and complexity of the Competition class by grouping the operations that report/provide statistics in a dedicated class. To remove the glue the first step is to extract an interface from the CompetitonStatistics class.
Note how we also have to pass in IEnumerable<Entrant> entrants via the constructor of the Competition class even though it was only required by CompetitionStatistics. This is known as tramp data which is defined as:
Data which is passed via one object to another, and not otherwise used by the first.
By extracting the interface the constructor now looks as:
public Competition(IPossibleAnswers possibleAnswers, ICompetitionState state, ICompetitionStatistics statistics)
{
CreationDate = DateTime.Now;
PossibleAnswers = possibleAnswers;
State = state;
Statistics = statistics;
}
NB we also followed the same process with the code: new PossibleAnswers().
This change creates a new problem in that the Competition class requires an instance of ICompetitionStatistics but our concrete implementation requires some data from the Competition class as detailed in the following constructor:
public CompetitionStatistics(CompetitionAnswer correctAnswer, IEnumerable<Entrant> entrants)
{
CompetitionAnswer = correctAnswer;
Entrants = entrants;
}
The reason for creating the separate CompetitionStatistics class was to prevent too many properties/methods existing in the Competition class. We were trying to comply with Code Complete's advice to try and limit a class size to roughly 7 data members. It's also often said of a large class that:
When a class has too many instance variables, duplicated code cannot be far behind.
But what's worse, having a class with a little over 7 data members or having separate classes with circular dependencies on each other? I think the latter is much worse, so I'm going to move the methods from the CompetitionStatistics class back into the Competition class. There is a lesson here in that it's easy to over complicate matters in the quest of simplicity. The fact that the readonly properties on this class could not work without the correct answer details from the Competition class tells us that this is where the logic should have been contained from the beginning.
Why it's a good idea to use an EntityBase
I had omitted a useful feature of domain validation. Each domain entity has an IsValid property that will examine its internal state and tell us whether the entity is valid or not. This was achieve by each domain entity implementing the following interface:
public interface IValidatable
{
bool IsValid { get; }
}
The missing feature is the ability to report what is making the entity invalid e.g. required fields, out of range values etc.
The solution to this was to create a new abstract class called EntityBase:
public abstract class EntityBase : IValidatable
{
private ValidationErrors _validationErrors;
public Guid ID { get; set; }
public bool IsValid
{
get
{
_validationErrors.Clear();
Validate();
return ValidationErrors.Items.Count == 0;
}
}
public ValidationErrors ValidationErrors
{
get
{
return _validationErrors;
}
}
public EntityBase()
{
_validationErrors = new ValidationErrors();
}
protected virtual void Validate() { }
}
Each domain entity can optionally choose to validate itself over overriding the Validate method, for example:
protected override void Validate()
{
if (string.IsNullOrEmpty(CompetitionKey))
ValidationErrors.Add("CompetitionKey", "There should be four possible answers");
if (Answer == Competitions.CompetitionAnswer.NotSet)
ValidationErrors.Add("Answer");
if (Source == EntrantSource.NotSet)
ValidationErrors.Add("EntryDate");
}
Fortunately I didn't have too many classes which I had to revisit to add the EntityBase inheritance, but next time I will be sure to add this in from the beginning for greater flexibility!
Are your unit tests crossing boundaries?
When reviewing a unit test I realised I had made a mistake with one of the tests. What's wrong with the following code?
[TestMethod]
public void CompetitionIsValid()
{
// Arrange
Competition competition = new Competition(new PossibleAnswers(), new OpenState());
competition.Question = "Who is Luke Skywalker's father?";
competition.ClosingDate = DateTime.Now.AddMonths(1);
competition.CompetitionKey = "WINPRIZE";
/*** START HINT ***/
competition.PossibleAnswers.Add(new PossibleAnswer() { Answer = CompetitionAnswer.A, Description = "Darth Vader", IsCorrectAnswer = true });
competition.PossibleAnswers.Add(new PossibleAnswer() { Answer = CompetitionAnswer.B, Description = "Obi Wan Kenobi" });
competition.PossibleAnswers.Add(new PossibleAnswer() { Answer = CompetitionAnswer.C, Description = "George Lucas" });
competition.PossibleAnswers.Add(new PossibleAnswer() { Answer = CompetitionAnswer.D, Description = "Walt Disney" });
/*** END HINT ***/
// Act
bool isValid = competition.IsValid;
// Assert
Assert.IsTrue(isValid);
}
The IsValid method calls Validate:
protected override void Validate()
{
if (string.IsNullOrEmpty(Question))
ValidationErrors.Add("Question");
if (string.IsNullOrEmpty(CompetitionKey))
ValidationErrors.Add("CompetitionKey");
if (ClosingDate == DateTime.MinValue)
ValidationErrors.Add("ClosingDate");
/*** START HINT ***/
if (!PossibleAnswers.IsValid)
ValidationErrors.AddRange(PossibleAnswers.ValidationErrors.Items);
/*** END HINT ***/
}
The problem is that we are building up the PossibleAnswer list to meet the requirements of the Validation implementation on that class, but we are only interested in testing the Competition class. There are separate unit tests that explicitly test the PossibleAnswers class. This not only clutters up the unit tests for the competition tests but we are also crossing a boundary and testing the implementation detail of the Validate method on both classes.
Earlier in this post we looked as extracting an interface from the PossibleAnswers which is something we can make use of. The test below has been reworked and we are now using a stub instance of IPossibleAnswers. Rather than having to setup our data to meet the functionality of the PossbileAnswers implementation we were using previously we can now setup this stub object to return the state we expect. This is a small change but it means we are no longer relying on the implementation detail of another class in order to make this test pass. Here is the reworked test:
[TestMethod]
public void CompetitionIsValid()
{
// Arrange
StubPossibleAnswers answers = new StubPossibleAnswers();
answers.SetValidState(true);
Competition competition = new Competition(answers, null);
competition.Question = "Who is Luke Skywalker's father?";
competition.ClosingDate = DateTime.Now.AddMonths(1);
competition.CompetitionKey = "WINPRIZE";
// Act
bool isValid = competition.IsValid;
// Assert
Assert.IsTrue(isValid);
}
// The stub implementation
private class StubPossibleAnswers : IPossibleAnswers
{
public IEnumerable<PossibleAnswer> Answers
{
get { throw new NotImplementedException(); }
}
public PossibleAnswer CorrectAnswer
{
get { throw new NotImplementedException(); }
}
public void Add(PossibleAnswer possibleAnswer)
{
throw new NotImplementedException();
}
public ValidationErrors ValidationErrors
{
get
{
if (IsValid)
{
return new ValidationErrors();
}
else
{
// add one row to put this in an invalid state
ValidationErrors errors = new ValidationErrors();
errors.Add(new ValidationError("property", "required"));
return errors;
}
}
}
public bool IsValid { get; private set; }
public void SetValidState(bool isValid)
{
this.IsValid = isValid;
}
}
Use Extension Methods to contain Generic Functions
I had a minor issue with the following method:
private int GetRandomEntrantIndex(CompetitionStatistics statistics)
{
return new Random().Next(statistics.CorrectAnswers.Count());
}
It's used by the OpenState() class as illustrated below:
winner.Entrant = competition.CorrectEntrants.ElementAt(GetRandomEntrantIndex());
The method was within the Competition class but it seemed a little random. It's a generic method so should I create a helper class? I tried to avoid helper classes as they generally turn into a dumping ground for random functions. Since this method can work with a "list of things" I opted to create an extension method named SelectRandom. Here is the code:
public static class IEnumerableExtensions
{
public static T SelectRandom<T>(this IEnumerable<T> source)
{
return source.ElementAt(GetRandomArrayIndex(source.Count()));
}
/// Gets the random index of the array.
private static int GetRandomArrayIndex(int arrayCount)
{
return new Random().Next(arrayCount);
}
}
What's best about this is the way the calling code now reads:
if (competition.HasCorrectEntrants)
competition.Winner = competition.CorrectEntrants.SelectRandom();
I would dare suggest this may even be a little elegant!
Unit Tests to the Rescue
The changes made as part of this review eliminated some code smells from the domain and allowed me to delete a few classes. The side effect being that I now had some broken tests. The unit tests I created as I drove the design, albeit with some mistakes, were priceless during this process. I was able to pin point exactly where I had introduced bugs and moved through each broken test until they were all passing. They offered me a form of a finish line so that I knew I was back to where I started in terms of correctness before I began making changes. Now I have all my tests passing and a much cleaner domain model! Oh, and my code coverage is now up to 88.40%!
Wrapping Up
Some of the changes made in this post are, micro-optimisations and being as our tests were working before and after the changes it meant that functionally we were meeting the requirements with both implementations. I think it's important to review such a simple application in such a manner, since if we cannot identify and correct these sorts of changes on a small scale then how can we do so with a big complex system! I deliberately left some issues in my code so that I could detail the concepts at this stage but I still came across unintentional mistakes.
The code for this post can be found at Github.
In the next post we will look at how we can use this domain model in our application. See the RSS subscription links at the bottom of the page to follow this series.
