Monday, September 26, 2011

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.

No comments:

Post a Comment