Answering to NHibernate codebase quality criticism
Patrick has a post analyzing the changes to NHibernate 2.1, which I take exception to. Specifically, to this:
Now, the code base is completely entangled and it must be a daily pain to maintain the code. There is a total lack of componentization and each modification might potentially affect the whole code base.
The problem with the way things are seen from Patrick’s point of view is that his metrics and the metrics used by the NHibernate team are not identical. As a matter of fact, they are widely divergent. This is part of the reason that I tend to use automatic metrics only as tracer bullets in most code review scenarios.
In particular, Patrick uses the notion of namespaces as a layering mechanism, when the codebase doesn’t follow that convention, the output is broken. NHibernate doesn’t use namespaces as a layering mechanism, hence, the results that Patrick is showing.
Just to give you an idea, we have tens of new features in the new release, and the number of issues have decreased. We are able to make changes, including major ones, with no major problems, and with safety. For that matter, I actually take offence at the notion that NHibernate isn’t componentized. It is highly componentized, and we are actually working on making this even more so, by adding additional extension points (like the pluggable proxies, object factories, and more).
There is no question about NHibernate being complex, but in the last year or so we have taken on several new committers, drastically increased the number of new features that we ship and have maintained a high code quality. For that matter, we were able to finally resolve some long standing code quality issues (three cheers for the death of the NHibernate.Classic.QueryParser, and many thanks to Steve Strong for the ANTLR HQL parser)!
There are a few other issues that I would like to point out as well:
Methods added or changed represent 67% of the code base!
A lot of those are the result of internal refactoring that we do, a good example is the move to strongly typed collections internally, or renaming classes or methods to better match their roles. Those are, generally speaking, safe changes that we can do without affecting anyone outside of NHibernate Core. That helps us keep the high level of code quality over time.
30 public types were removed which can then constitute a breaking change.
Again, here we have a more complex scenario than appears upfront. In particular, NHibernate uses a different method for specifying what a breaking change is. By default, we try to make NHibernate extensible, so if you need to make changes to it, you can do that. That means we make a distinction between Published for Inheritance, Published for Use and Public.
For example, we guarantee compatibility for future version if you inherit from EmptyInterpcetor, but not if you implement IInterceptor. By the same token, we guarantee compatibility if you use ISession, but not if you try to implement it. And then there are the things that are public, extensible by the user, but which we do not guarantee which be compatible between versions. Dialect are a good example of that.
This approach means that we have a far greater control over the direction of NHibernate, and it has stood for us throughout 4 major releases of the project.
As for how we measure code quality in NHibernate? We measure it in:
- Peer code review (especially for things that are either tricky or complex)
- Reported bugs
- Ease of change over time
- Quality of deliverable product
So far, even if I say so myself, I have to say that there is good indication that we are successful in maintaining high quality codebase. Complex, yes, but high quality, easy to change and easy to work with.