Terse Testing

I was writing some tests recently and thought, "This is ugly to read and hard to write":

Here is how I rewrote it:

This is much more readable than the verbose version. When you're writing more than a few of these tests, it really pays off in writability as well. I called over a co-worker for a quick code review and he approved.

It might seem "bad" or "unsafe" to use strings like this instead of staying under the compiler's jurisdiction. And what about refactoring? Well first of all, these are tests. If they break, you will notice it. But second of all, I think the terse version is just as solid as the verbose version. Since the mapping from one representation (array of nullable ints) to another (a string) is both trivial and bidirectional, being coupled to one representation is pretty much the same as being coupled to the other representation.

I had actually used this technique quite a while ago in a falling block game (in the style of Dr. Mario or Puyo Puyo). I wrote tests that used an ASCII-art representation of the game grid, which worked just fine. Tests written directly against the data structures were difficult to write, and almost impossible to read.

Code Review Checklist part II: Overloaded Intent

Part 1 is here

I'll describe another mistake in hopes that I won't make it again. But this one is pretty subtle and hard to catch. Here's some code:

var init = new Initializer();

// This is a facade over the composition root.
// You can configure settings, register plugins, etc. For example:
init.RegisterPluginSource(@"C:\Plugins");

init.Initialize();

I was using this in two applications and a handful of tests. I had just refactored it, and compared to what the code used to look like, this is pretty DRY and I was feeling good about it. But if I had been thinking a little harder I might have noticed a problem waiting to happen.

Checklist Item: Beware of Overloaded Intent. If a method or type is being depended on in multiple places, make sure the intent is similar enough.

I made some changes to the Initializer and my tests started failing! The problem was that I was now registering some IStartables– they were running during tests eating data out of producer-consumer queues. I didn't want to force the applications to explicitly enable IStartables, nor did I want test authors to explicitly disable them. The real problem was that I was using the new Initializer() constructor with two different intents – one to wire up a production system and another to wire up tests. It was easy enough to change all the tests to use a separate method like BuildInitializerForTesting(), but it could have been more painful if there had been a huge number of tests.

Overloaded intent is a latent problem – some method or type is serving multiple intents and everything is fine until something changes and now the method cannot support both intents. It's a lot easier to spot duplicate code than it is to spot overloaded intent. In fact, creating and using a method M2 that does nothing but call M1 can look pointless. The same goes for creating an interface I2 that matches I1 perfectly. But it's not pointless if M2 is promising something different than M1, even though their implementations are currently identical. When calling a method, think "OK, this method does what I want. But is that a coincidence or a promise?"

It can be difficult to anticipate these divergences before they happen, but it's still worth it to look out for. Keep an eye out for methods, interfaces, and classes that are used in many places – the more something is being used, the more likely it is being used with overloaded intent.

Code Review Checklist part I: Method Signature Coupling

I recently found and corrected two interesting mistakes in my code. I don't know how they got committed in the first place because my code review checklist is supposed to catch them. I guess all I can do is write about it, so I'll remember next time. Maybe you'll find it useful too.

The Problem

I had an interface method defined like this:

FieldBinding Resolve(Field field, string systemName)

I remember thinking "I sure hope that's enough information for implementations to do the work they need to do." Specifically, systemName is unused at the moment but I put it in because there is required functionality that I know will need it. But when I get around to implementing it, will systemName be enough, or will I need even more information? As as I started implementing this interface in multiple places I realized that there's a big problem with the way it's defined.

Checklist Item: Beware of Method Signature Coupling. For any method that will be implemented or overridden in more than one place, make sure the method signature is maintainable enough.

If the signature of Resolve changes, all the implementations I was writing would need to change with it. So I changed it to this:

FieldBinding Resolve(ResolutionArgs args)

Much better. Now that method signature is a lot more change-resistant. If I need to add more context, I can add it to the ResolutionArgs. Another good sign is that I can get rid of the currently-unused systemName. When someone implements that functionality, they can decide what data is actually needed and add it to the ResolutionArgs without breaking existing code.

I remember this as the “MouseEventArgs Rule” probably because this is where I first noticed this pattern and thought about the benefits. But that's not a good name for the checklist item because an “args-style” class is just one solution to the problem of being coupled to a specific method signature. I don't automatically introduce an “args-style” class when the checklist item raises its warning flag – it's just a code smell that indicates some form of refactoring may be needed.

I actually noted this as a code reviewer a while back. The code under review had a BaseViewmodel that took 3 constructor arguments. This was a plugin-driven system where there would be many plugins inheriting from BaseViewmodel. I noted that changing the BaseViewmodel's constructor would break all the subclasses and suggested, among other things, encapsulating the dependencies into a single “args-style” class. So why didn't I catch it when I was the one making the mistake?

Part 2 with Mistake 2 is here

Simple Test Utils

When I'm writing tests, I prefer not to use a mock/stub/fake library until simpler methods become painful. Here are two functions I find myself using in many of my tests to break encapsulation and arrange the test.

Set a property with a private setter

public static void SetProperty<T, V>(this T obj,
    Expression<Func<T, V>> expression, V value)
{
    var body = expression.Body as MemberExpression;
    if (body == null)
    {
        throw new Exception("expression.Body must be a Property");
    }

    var property = body.Member as PropertyInfo;
    if (property == null)
    {
        throw new Exception("expression.Body must be a Property");
    }

    property.SetValue(obj, value, null);
}

Usage:

var entity = new SomeEntity();
entity.SetProperty(me => me.Id, 123);

I like the Intellisense support when choosing the property, and I especially like the type safety here – passing "a string" to an integer property is a compile-time error.

Instantiate an object with no public constructor

public static T Instantiate<T>() where T : class
{
    return System.Runtime.Serialization.FormatterServices
        .GetUninitializedObject(typeof(T)) as T;
}

Usage:

var entity = Instantiate<SomeEntity>();

This one you have to be a little more careful with. It will not call any of the constructors of the class. If you just want an object to exist, this method is fine. If you want an object to exist in a certain state, then you may want to use reflection to call a specific constructor. Relying on the existence and behavior of a particular constructor is more brittle than GetUninitializedObject with regard to mere instantiation. With regard to initialization, it depends on the type of object being created, and what you need from that object. For example, if all you're going to do is call object.GetType(), then it doesn't matter if the object was properly initialized.