NDepend Maintainability Metrics

Now, I have some contempt for code metrics, ever since I discovered that TFS thought that my worst code was a constructor that checked its parameters for null.  Metrics are useful indicators.  Profit is a metric used to measure the health of a company.  It’s not always useful, but that’s the nature of metrics, they’re indicators, not hard and fast rules.  I thought it might be interesting to examine Patrick’s original post about NHibnerate 2.1 which started the whole recent argument about maintainability.

First, he took a look at the number of changes to the code base.  There’s been a phenomenal amount of work done here, and I don’t regard that as a problem.  It still passes pretty much the same unit tests.  Replacing a dreadful piece of code with a good piece of code that satisfies the same conditions doesn’t make it less maintainable, it just means Steve Strong is smoking.  However, metrics are pointers, and the sheer number of places changed by the HQL changes suggests the possibility that the old HQL code was a mess.  This isn’t necessarily the case, and I’m not qualified to judge, but that is what the metrics are suggesting.

Next, he took a look at the assembly dependencies.  Here the story is that the code base is improving: the dependencies on Castle have been removed, part of the project to allow pluggable interceptors, and a dependency on ANTLR3 has been taken.  This isn’t a problem: it doesn’t introduce an externality and it doesn’t affect user preferences: no-one’s about to demand a different HQL lexer, but plenty of people wanted to plug in their own proxy framework.

Entangled Code

Okay, now we get to something interesting: Entangled Code.  Patrick’s been pushing the idea that you shouldn’t have namespace circularities for quite some time.  It’s not standard practice (Udi Dahan does it in nServiceBus, albeit in a bizzare and inconvenient way), but actually it’s probably a good idea.  I think it’s an idea that would gain more traction if an open source tool integrated the concept directly into Visual Studio. 

But entanglement at the class level?  As Ayende points out, it’s often the only way you can get things done.  No-one worries about circular dependencies with inner classes, and they shouldn’t.  Since the difference between an inner class and an external class is often file size, I think a two element cycle is completely ignorable.  On the other hand, some of the dependencies are solvable:  the dependency of SessionImpl on SessionFactoryImpl, for instance, is only really required by serialization.  This is solvable.  Whether you regard it as a problem is quite another matter.  With my purist hat on, I’d certainly prefer it was separated out, but I’m sure many would argue it doesn’t much matter.

The truth is: no-one’s going to address this in a hurry.  Whatever the costs of having your code all over your namespaces, it’s probably smaller than the cost of diverging too far from the Hibernate source base.

Breaking Changes

Well, first let me say a breaking change is a breaking change.  On the other hand, I’m not sure I’d be too bothered about

• Removed public types that had no public constructors and were never returned with their concrete type
• Changing interfaces:  interfaces are brittle, abstract classes aren’t.  We know that.  It’d be nice if there was a public document that explained NHibnerate’s upgrade policy, but I’m not going to lose sleep over this.

There’s a question of ALT.NET philosophy here.  If you really wanted to avoid breaking changes, you’d use abstract classes for everything just like Microsoft.  The cost of the occasional breaking change isn’t really that high.

Truth is, if you drill in, Oren’s right not to worry.  Most people are going to drop in the new assembly and not notice the difference.  The one thing they’re really going to notice, the requirement to specify a ByteCodeProvider, is completely intentional and isn’t picked up by any of these metrics.  Most of the other “breaking changes” aren’t going to break real code.  NHibernate actually has a very strong story here: the release notes highlight all of the likely breaking changes.

Code Metrics

If you actually take a look at Patrick’s report on “methods that could do with refactoring”, you’ll see that a particularly large offender is one of SessionFactoryImpl’s constructors.  Personally, I think that many code metrics don’t take constructor injection into account:  there’s certain patterns of behaviour you expect to see in a constructor that send cyclomatic complexity through the roof.  It doesn’t do the code any harm, though.

Measures such as cyclomatic complexity sometimes punish desirable behaviours.  It’s time we got better metrics.  For example, I don’t believe checking a parameter for null and then throwing an ArgumentNullException should make a code quality metric look worse.

A Bad Example

NHibernate was picked by Patrick for this analysis because it’s a large, well-known open source code base.  However, unfortunately it’s a singularly bad code base to pick for this kind of analysis.  Like it or not, NHibernate’s maintainability is directly affected by its similarity to the Hibernate code base.  A metrics-driven refactor would actually decrease project velocity, which is ultimately all we care about.