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