‘Uncle Bob’ wrote the following in Clean Code: A Handbook of Agile Software Craftsmanship:
“The ratio of time spent reading (code) versus writing is well over 10 to 1 … (therefore) making it easy to read makes it easier to write.”
So think about that when you write your next bit of code. You need to make sure your code is easily readable and understandable for others, you hardly ever write code just for yourself.
Unit test code is not different than ‘production’ code. Readability is key here because unclear unit tests will be distrusted, ignored and possibly removed.
Consider the following unit test for testing the
GetHighestRatedMovies method in an imaginary
Although this unit test uses some great frameworks such as xUnit, AutoFixture, FakeItEasy and FluentAssertions (big fan of these!) there are still some things I don’t like, particularly in the Arrange section:
- In lines 5-6 a collection of
Movieobjects is set up using AutoFixture. I get why this collection is necessary but I really don’t care about how it’s done. In addition, you could argue that 20 is a magic number although the intent is quite clear here. It can definitely be a bit more clear.
- In lines 7-8 a fake object based on
IMovieRepositoryis created using FakeItEasy. The fake repository is required to be able to return
Movieobjects from the
MovieServicebut again I don’t really care how that is done.
- On line 9 the
MovieServiceis instantiated and the fake repository is passed in the constructor. But what are the
nullarguments there? What do they represent?
- On line 10 a
MovieServiceRequestobject is constructed. It’s a simple value object with just one property. But what will happen if more properties are added later? Then the construction of this request will take up quite some space which has a negative impact on readability.
In general, I feel there is too much detail in this Arrange section which is not relevant for understanding the unit test. Although 6 lines is not that much I do believe fewer lines in the Arrange and clear usage of arguments will improve the readability a lot.
Here’s how I refactored the unit test:
There are two major differences:
- Creation of fake objects and the request object is now done in private methods. This allows the re-usage of these methods in future unit tests. When the number of these helper methods grows you should consider moving them to a separate class.
Named arguments (available since C# 4.0) are used when calling the helper methods and for constructing the
MovieService. It is now evident what the
nullvalues actually represent. An alternative would be to declare separate variables for all these arguments. Although that would be very clear and descriptive I believe that would hurt readability because the Arrange section would get quite bulky again.
- I usually only ‘new up’ the class under test directly in the Arrange section, other (fake) objects are created in helper methods or classes.
- Use existing and proven libraries & frameworks so you don’t have to write boilerplate code and you can focus on more difficult problems.
- Try to use a mocking framework (such as FakeItEasy) over a custom made mock/stub framework. Using a custom framework costs time in two ways: it needs to be maintained and it needs to be explained to every new member on the team.
- Need to create collections of fake objects? Consider using AutoFixture’s
- The FluentAssertions library contains dozens of useful assert methods, especially for collections.
- Always be very explicit in naming the methods and arguments to avoid unclarity. Consider using named arguments when you need to pass numbers, strings or null to methods. If you’re using code analysis tools such as Resharper you’ll get warnings that the usage of named arguments is not required most of the time. You might want to lower the severity of that message so the code analysis stays ‘green’.
Happy unit testing!