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