Pet Peeve #726

Alright, so this is a weird thing to get me blogging again, but it really annoys me.

In English, the -ing suffix is reserved for verbs. 

Simple as that, really. If you want to create a verb out of a noun, you can add -ing. Appropriately it’s called verbing.

It really jars when I see a noun with the -ing suffix (words like “thing” don’t count, obviously).

This means that there is a no such thing as a refactoring.

“To refactor” is the verb. Therefore, “refactoring’ is something you do. You can do a single refactor. You cannot do a single refactoring. You can do multiple refactors. You cannot do multiple refactorings.

Also… Advice is like sheep – it’s the same whether singular or plural. In AOP, you do not apply ‘advices’, you apply ‘advice’ even if you happen to be applying many of them.

</pedantry>

Put boolean arguments last

Whenever I see a method call like this:

Enum.TryParse(stringRepresentationOfEnum, true, out enumValue);

I wonder what the `true` parameter really represents. In this instance, because I know that method from past experience, I know that this is the ignoreCase parameter.

However, what if this is a totally different method that you’ve never seen before:

service.DoOperation(DateTime.Now, true, false, false, true, currentValue, userName);

This is a particularly sadistic method. But let’s assume that this is the necessary signature of the method. Sadly, it is very unclear what the boolean values do that are being passed in here. What options do we have for adding clarity, assuming we can’t alter the interface?

Firstly, we could pass in some named variables for each of the boolean values:

bool includeHeader = true;
bool checkSecurity = false;
bool validateDate = false;
bool fireCompletionEvent = true;
service.DoOperation(DateTime.Now, includeHeader, checkSecurity, validateDate, fireCompletionEvent, currentValue, userName);

This is already much better, but it is untidily verbose. While we have much more clarity as to what is being passed into the method, it comes with a bit of noise in having to declare the parameters. This feels especially redundant because the variable names will likely be the same as the formal parameter names. As of C# 4.0, we need not do this. Instead, we can use named parameters:

service.DoOperation(DateTime.Now, includeHeader: true, checkSecurity: false, validateDate: false, fireCompletionEvent: false, currentValue, userName);

Sadly, this doesn’t quite work as hoped- the last two parameters are `positional` parameters and cannot appear after named parameters. This means that we must introduce some redundancy:

service.DoOperation(DateTime.Now, includeHeader: true, checkSecurity: false, validateDate: false, fireCompletionEvent: false, currentValue: currentValue, userName: userName);

This is why I would advocate putting boolean values last in the formal parameter list:

service.DoOperation(DateTime.Now, currentValue, userName, includeHeader: true, checkSecurity: false, validateDate: false, fireCompletionEvent: false);

This is much clearer, but it does require you to design your method signatures with this in mind.

Towards better unit testing organization…

I try to follow a test-first approach at all times. However, the way in which I organize my unit tests has evolved over the past five years or so.

Originally, I would follow the pattern espoused in unit testing tutorials:

[TestFixture]
public class CalculatorFixture
{
	[Test]
	public void CanAddTwoPositiveNumbers()
	{
		var calculator = new Calculator();
		int result = calculator.Add(13, 45);
		Assert.AreEqual(58, result);
	}

	[ExpectedException(typeof(OverflowException))]
	[Test]
	public void OverflowCausesException()
	{
		var calculator = new Calculator();
		calculator.Add(int.MaxValue, 1);
	}
}

This is pretty much as simple as it gets. However, it is time-consuming and does not promote reuse of repeated code. While we can move initialization into methods marked [SetUp] this will be run for every test in the fixture, which might not be suitable.

So, I factored things out into a proper Arrange, Act, Assert pattern. I forget where I picked this up from, but – as with many things – I spotted it, briefly evaluated it and decided to run with it.

[TestFixture]
public abstract class TestBase
{
	[SetUp]
	public void Init()
	{
		GivenThat();
		When();
	}

	protected virtual void GivenThat()
	{
	}

	protected abstract void When();
}

This is still fairly simple, but much more expressive. It allows me to specify my tests as follows:

[TestFixture]
public abstract class WhenUsingTheCalculator : TestBase
{
	protected abstract override void GivenThat()
	{
		base.GivenThat();
		this.calculator = new Calculator();
	}

	private Calculator calculator;
}

public abstract class WhenAddingTwoNumbers : WhenUsingTheCalculator
{
	protected override void When()
	{
		this.result = this.calculator.Add(X, Y);
	}

	protected abstract int X { get; }

	protected abstract int Y { get; }

	protected int result;
}

public class WhenAddingTwoPositiveNumbers : WhenAddingTwoNumbers
{
	protected override int X { get { return 13; } }

	protected override int Y { get { return 45; } }

	[Test]
	public void ItShouldReturnTheCorrectResult()
	{
		Assert.AreEqual(58, this.result);
	}
}

public class WhenAddingNumbersThatCauseOverflow : WhenAddingTwoNumbers
{
	protected override int X { get { return int.MaxValue; } }

	protected override int Y { get { return 1; } }

	[Test]
	public void ItShouldThrowAnOverflowException()
	{
	}
}

This style isn’t much of an improvement, to be entirely honest. Sure, it generates a nice output when a test fails (“WhenAddingNumbersThatCauseOverflow.ItShouldThrowAnOverflowException() failed”). Note that the latter test won’t work, though. More work is required to allow expected exceptions to be caught (hint: the exception is thrown in When() and the [Test] methods are run after this).

I persevered with an enhanced version of this for a while though, because the reuse seemed worth the pain of such a leaky abstraction. But, the fact that the reuse was through inheritance ended up causing a bit of a problem, because anyone who came to the tests after they were originally written were flummoxed by the complex, deep hierarchies. Sometimes this person was me…

I recently decided to go back to the drawing board to try to find something that would hit the following requirements:

  • Support the reuse of initialization code and assertions
  • Rely more on composition than inheritance for creating tests
  • Be really easy to understand for anyone seeing tests for the first time (tests are a reliable form of documentation)
The result is as follows:
Unit Testing domain

First, a quick explanation. A unit test has very simple behavior: Arrange() the preconditions, Act() on the target object, Assert() the postconditions. This is probably all a test runner will need to know about a unit test (if that, it could probably just ‘Run()‘ unit tests, but by splitting them into constituent parts, we could try/catch around only the Assert() looking for AssertionExceptions, for example).

Where data is concerned, a unit test is composed of many initializers, many assertions and one action. Notice that each of these components do not exclusively belong to a single unit test instance, but are shared among them, promoting reuse.

Implementing this model quickly showed how far you can stray from documentation in just a short while. I’m going to skip posting and explaining the implementation (but I’ll supply a link at some point with the code when I’m happy with the result). Instead, I’ll share the bit that really matters: how it looks to clients.

Under my CalculatorTests project I have one file called Initializers.cs (for all Calculator preconditions) one file called Assertions.cs (for all Calculator assertions) and then a file called CalculatorTests.cs which contains all of the unit tests pertaining to the Calculator class. Note that this is the only file that people need look in to discern the intent of the code.

Initializers:

public class CalculatorIsDefaultConstructed : IInitializer<ICalculator>
{
	public void Prepare(ref ICalculator target)
	{
		target = new Calculator();
	}
}

Assertions:

public class ResultShouldEqual : IAssertion<ICalculator, int>
{
	private int expectedValue;

	public ResultShouldEqual(int expectedValue)
	{
		this.expectedValue = expectedValue;
	}

	public void Verify(ICalculator target, int returnValue)
	{
		Assert.IsNotNull(target);
		Assert.AreEqual(this.expectedValue, returnValue);
	}
}

public class ExceptionThrown<TException> : IAssertion<Exception>
	where TException : Exception
{
	public void Verify(Exception exceptionThrown)
	{
		Assert.IsInstanceOf(typeof(TException), exceptionThrown);
	}
}

The unit tests now look like this:

public class CanAddTwoPositiveNumbers : UnitTest<ICalculator, int>
{
	public CanAddTwoPositiveNumbers()
	{
		GivenThat<CalculatorIsDefaultConstructed>();
		When(calculator => calculator.Add(13, 45));
		Then<ResultShouldEqual>(58);
	}
}

public class OverflowCausesException : UnitTest<ICalculator, int>
{
	public OverflowCausesException()
	{
		GivenThat<CalculatorIsDefaultConstructed>();
		When(calculator => calculator.Add(int.MaxValue, 1));
		ThenThrow<OverflowException>();
	}
}

So, each unit test has its Arrange, Act, Assert components clearly visible as well-named classes, but the implementation of each is hidden away elsewhere. It can be inferred what CalculatorIsDefaultConstructed does, so you don’t need to see its guts. The almost English-language specification of the test is quite nice, too. I’m going to add a fluent interface to the initialization and assertion registration, to include And(). This is now, for all intents and purposes, Behavior Driven Development (BDD)…

There are a couple of issues with this approach, still. Mainly, it is overkill for the example presented here. The initializers and assertions made aren’t complex enough to warrant their own classes and the readability of the intent isn’t sufficient reason to justify the extra fingerwork.

I’m about to write a non-trivial suite of unit tests using this organization and I will report back with my findings and – by then – some code. The implementation of UnitTest is integrated into NUnit so I can use its framework wherever required.

How do you organize your unit tests?

Do you really need an ORM?

An Object-Relational Mapping (ORM) is necessary only when you have 1) an object model, and 2) a need to persist this object model to a Relational Database Management System (RDBMS).

The latter may be a given, especially in web-based scenarios. However, the former is not always so clear-cut.

You have an object model if you have a collection of fully-fledged classes with both data and behavior.

Object Oriented Design (OOD) is predicated on the tight coupling between an object’s data and the methods that operate on that data. Without the associated behavior you are left with an anemic domain model. What you have is an object graph with relationships and data that map to the problem domain, but the fine-grained behavior is limited to getters and setters for the data.

A simple Blog class diagram; note the lack of object behavior

Often, the ‘business logic’ is extracted into a service layer which orchestrates the objects at a much more coarse-grained level.

The UML diagram shown is a very simple blog class diagram. Notice that there are objects, with names matching the nouns in the problem domain. However, there is no behavior on any of the objects. User is missing verbs like Login, CreateCategory and SubmitComment. Post is missing verbs like SaveDraft and Publish. Comment is missing verbs like Accept and Flag.

These are the fine-grained behaviors that will probably end up rolled into a service layer’s higher-level functionality. There is nothing wrong with this, providing that there is not an ORM sitting between this object graph and persistent storage (as opposed to just serializing the object graph).

As blogs are typically online, it is reasonable to assume that there would be an RDBMS used as the persistent storage. This covers one half of the prerequisites of using an ORM. But there is definitely no need for an ORM here because there is no domain model.

So, what are the alternatives?

Martin Fowler provides a number of alternatives in his book Patterns of Enterprise Application Architecture – under Data Source Architectural Patterns.

Rather than enumerate the high-level patterns that Martin Fowler has already suggested, I’ll jump into technology choices instead. Assuming that the ORM of choice was originally NHibernate, you could just choose a simpler ORM such as Entity Framework. This will certainly make generating the mapping code far simpler than learning NHibernate’s configuration files. But, even that could be argued as overkill.

Perhaps Active Record would be a good fit, but again this is also a suitable alternative when there is behavior inside the objects, thus potentially overkill.

The simplest solution that I would advocate when there is no behavior in the domain model is a Typed DataSet. First of all, a typed dataset can be generated from an existing database and this yields the CRUD operations for all of the tables, as well as locally maintaining referential integrity without having to hit the database again.

Furthermore, if deemed necessary, the messy auto-generated code can be hidden behind neat interfaces for each domain object by adding a partial definition for each class generated – as long as the properties match the type and name of those generated.

What we are creating with this pattern is merely Data Transfer Objects (DTOs) – which is pretty much what we started with because DTOs are objects without any behavior. We’re just admitting the fact and making design decisions based on that admission.

Perhaps you may think that there will be a future requirement for behavior in your domain model which merely hasn’t yet been fulfilled. Perhaps, but until that day comes, you would save a lot of effort by starting with the simplest thing that works.

Skyhooks vs Cranes

Daniel C. Dennett wrote in his 1995 book Darwin’s Dangerous Idea [emphasis author’s]

“A skyhook is … an exception to the principle that all design, and apparent design, is ultimately the result of mindless, motiveless mechanicity. A crane, in contrast, is a subprocess or special feature of a design process that can be demonstrated to permit the local speeding up of the basic, slow process of natural selection, and that can be demonstrated to be itself the predictable (or retrospectively explicable) product of the basic process.”

Daniel C. Dennett, Darwin’s Dangerous Idea,1995 (p76)

Basically, a skyhook is a way to explain something without reference to a prior antecedent. Conversely, cranes have explicable antecedents – perhaps until arriving at some primary axiom.

This is a useful analogy in programming, too. Skyhooks are a code smell; indicative of a deeper problem. All skyhooks should be replaced with appropriate cranes.

A skyhook makes your code difficult to mock. Examples of skyhooks are:

  • Static methods
  • Singletons
  • Object construction using new
  • Extension methods
Each of these make testing more difficult* by hindering your ability to inject mocks into your code, they are skyhooks and thus they are undesirable. Each is used ex nihilo – from nothing.
Thankfully, each of these can be replaced by a suitable crane which will facilitate some kind of external injection (ie: used ex materia – from something).
  • Interfaces
  • Dependency Injection
  • Inversion of Control
  • Factories
Let’s take an example of each skyhook and change it so that we use hooks instead.
public class UsersModule : IModule
{
	public void Initialize()
	{
		var unityContainer = ServiceLocator.Current.GetInstance<IUnityContainer>();
		unityContainer.RegisterType<object, UsersDocumentContent>(RegionNames.ModuleDocumentContentRegion(ModuleName));
		this.currentUser = new User();
	}
}
This method contains all four of the aforementioned skyhooks (see if you can spot where before continuing).
Firstly, the use of ServiceLocator is an example of why the Singleton pattern is my pet hate. I firmly consider it an anti-pattern.
Granted – in this instance – the ServiceLocator has a SetLocatorProvider method which unit tests can use to inject the locator, this is a less-than-desirable way of giving a class a service locator. Furthermore, you’ve now given this code the keys to the safe with regard to what services it can acquire. I’d far rather be explicit with the dependencies that I a class requires:
public class UsersModule : IModule
{
	public UsersModule(IUnityContainer unityContainer)
	{
		this.unityContainer = unityContainer;
	}

	public void Initialize()
	{
		this.unityContainer.RegisterType<object, UsersDocumentContent>(RegionNames.ModuleDocumentContentRegion(ModuleName));
		this.currentUser = new User();
	}

	private IUnityContainer unityContainer;
}
Now, the specific dependency has been constructor-injected, allowing unit tests to provide a suitable mock.
The second and third skyhooks are similarly extracted:
public class UsersModule : IModule
{
	public UsersModule(IUnityContainer unityContainer, IRegionNameProvider regionNameProvider, IUserFactory userFactory)
	{
		this.unityContainer = unityContainer;
		this.regionNameProvider = userFactoy;
		this.userFactory = userFactory;
	}

	public void Initialize()
	{
		this.unityContainer.RegisterType<object, UsersDocumentContent>(this.regionNameProvider.ModuleDocumentContentRegion(ModuleName));
		this.currentUser = this.userFactory.CreateNew();
	}

	private IUnityContainer unityContainer;
	private IRegionNameProvider;
	private IUserFactory userFactory;
}
I wouldn’t worry about the three constructor parameters – they’ll be injected by your IoC container.
The final skyhook, the RegisterType extension method, is a specialization of the static method skyhook (inasmuch as extension methods are implemented using static methods, an unfortunate design decision by Microsoft). Thankfully, extension methods must operate only on the public interface of the extended interface. This means that the call will likely result in an equivalent call to a method on the interface that was injected.
In conclusion, whenever you spot a skyhook, replace it with a crane.
* Some mocking frameworks, such as TypeMock, are able to mock skyhooks. However, this should only be considered if the skyhooks are in 3rd-party, unchangeable, code.