Maintainability: I think there’s something in the water

Did someone declare it “talented developers talk rubbish” week when I wasn’t paying attention?  Maybe I wasn’t on the mailing list… *sigh*

First we had the extremely smart Frans Bouma talking about the importance of proving your algorithms as a development methodology, which at least had the virtue of being funny.  We then had one of the single most productive developers in Alt.Net talking absolute garbage about maintainability.  At least Patrick Smacchia is still talking sense.  Let’s go back to what Ayende’s saying. 

Maintainable is a value that can only be applied by someone who is familiar with the codebase.

This is the exact defence I have seen of most of the worst systems I have ever encountered, from dodgy spreadsheets to distributed Access macros to over-engineered C++.  I have personally, to my shame,  delivered systems that had exactly these problems: they were well architected (to my definition of a good architecture), they were easy to modify (by me), followed consistent conventions (known to me).  That’s not a good system.  Even if you replace “me” with “us” you haven’t got a good system.  This is what I would term the “I’m incredibly smart” anti-pattern.

The problem with the “I’m incredibly smart” anti-pattern is: often the projects using it have incredibly high velocities.  Until someone leaves.  Companies often don’t have too much of a problem with this, especially during a recession.  Also, they know that the next bunch of developers who work on it will probably want to either a) rewrite everything or b) run the project in maintenance mode until it’s as bad as any legacy system.  The good news is that the latter scenario definitely isn’t happening to NHibernate.

Part of the problem is that maintainability should be a concept that is separate from the IQ of the developers:  NHibernate is developed by extremely smart people, that shouldn’t have any impact on its maintainability as a code base.  However, this doesn’t mean that Joe Graduate on his first coding job should be able to pick up and modify the code.  Maintainability isn’t a case of the lowest common denominator.  If so, we’d have to ensure we never used the following:

  • yield return
  • lambda expressions
  • LINQ
  • and frankly, NHibernate

I’ll talk a bit more about the middle ground in a later post.

Technorati Tags: ,

Don’t register controllers as singletons in your container

This is just me documenting a particularly weird behaviour of ASP.NET MVC.  Usually, it doesn’t matter, but if you used your own controller factory, like with MvcContrib, it’s a pain to diagnose.  Basically, if you re-use a controller, the ModelBindingContext’s values are cached.  This is, to say the least, freaky, especially since you can see that the url has changed.  So, you fire up one page, move to another id and get back the first page.  In short, register controllers as transient explicitly.  If you’re wondering how I found this out, consider that AllTypes in Castle Windsor registers services as singletons by default. 

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.

 

A New Subtext Skin

If you’re actually looking at this page on the web, rather than viewing it in Google Reader as my stats say is more likely, you’ll have had a bit of a surprise yesterday.  This is why I’ve been so quiet recently: I estimate that I’ve spent something like four man days on this skin and I’ve got to admit I’m quite exhausted.  Although Origami is a good professional skin, half of the .NET internet is using it and my customizations probably weren’t noticeable to most of the users of the site.

Let me set out what I wanted to achieve with the redesign:

  • I wanted to make the text easier to read.
  • I wanted to demonstrate the use of CSS to produce a “designery” look, without using Photoshop.
  • I wanted it to look good in all major browsers, but I wasn’t too fussed about pixel accuracy.
  • I wanted to try to emphasize engagement.  So, comment and feed subscriptions are now more prominent.

How it was done

This was developed in the classic fashion of ripping off the Naked skin.  Now, you’ll know from my previous posts that I’m not a designer.  However, this time I had help.  A lot of help, actually

  • Colour Scheme Designer is a god send.  Any problems you have with pink are with me, not the tool.
  • Equally, Pattern Cooler is a phenomenal site which does something too few “helper” sites do: work with you to produce the effect you’re after.
  • Gradient images and the bullet points were generated using the first tool I found on the net.  Although it’s basic, many of the others are worse…
  • Rounded corners were achieved using Rick Strahl’s CSS3 code.  It doesn’t work in IE7.  I don’t care.
  • Tim Heuer’s original Origami skin was an invaluable resource for figuring out “just how do you achieve that”.
  • I don’t have a good reference for the alpha blending, so I’ll write that up separately, but it’s another CSS3 feature
  • The menu bar is pure CSS, with a little help from some 1px wide gradient images.
  • I don’t think I’d have ever got live comment preview working without this post.  Which is ironic, because it’s obvious now.
  • Lea Alcantara‘s recent article for a list apart was fascinating.  I have to admit, I tried Trebuchet for the main text, but went back to Verdana.
  • Highlights are often achieved using CSS text-shadow, which has the advantage over changing the font that it doesn’t affect layout.
  • Gareth Slinn solved a particularly nasty and portable bug in dealing with comments.  If you want to know about accessibility, he wrote the book.

Thanks should also go to the Finn Brothers and Thelonious Monk, without whom I seriously doubt I’d have stuck at it.

There’s still plenty of bugs and rough edges, but I thought it about time I released:

  • It doesn’t look quite right in IE7.  I know, I should spend more time with it.
  • The code sections should be completely opaque, but they’re not, which is impeding readability.
  • The RSS icon doesn’t blend into the background.  I suspect that I’ll just live with that: it’s that or learn Photoshop.

All in all, Subtext skinning is an amazing demonstration of just what is possible with ASP.NET data binding if you’re committed.  I’d still prefer a pull model where the skin was given an API for rendering the page, though.  Certain things, such as customizing the date format, are pretty much impossible given the current code base, which is a pity.  I imagine Phil has this planned for version 3…

However, the more time I spend with Subtext, the more impressed I am.  It makes writing a blog engine look easy, and the more you learn about it, the more you realize it’s really not.  There are far more pages in Subtext than you might think at first.  I may still have missed one…  or two…

Anyway, I hope you like the new look.  I’ll get away from meta and back to content tomorrow, I promise.

Technorati Tags: ,,

NHibernate Code Quality Measures

I’ve already talked about Patrick’s measures of code quality.  His approach is to take a very direct mathematical and analytical approach to it.  I appreciate the sophistication of the tools he’s using, but I think that the results of this kind of analysis need to be treated with the same level of suspicion as an analyst treats a set of accounts.  e.g. Cash flow is hard to fake, but why are the accruals so low?

Ayende, on the other hand, argues for a very different approach.  He lists as his measures of code quality:

  • Peer code review (especially for things that are either tricky or complex)
  • Reported bugs
  • Ease of change over time
  • Quality of deliverable product

Let’s be frank, only one of these is actually a metric: reported bugs.  The others are a bit confused.

Peer Code Review

Let’s talk about peer code review.  Peer code review is absolutely best practice, it’s a cracking and important way of improving code quality.  But it’s a measure, it’s a practice.  Now, there are measurable things that we can ask about the reviews:

  • How often are code reviews carried out with developers in the same room?
  • How often do code reviews identify a bug?
  • How often do code reviews result in a change being rethought?

Even after that, you’d need to be very cautious about what you do with the metrics: a 0% defect rate and a 100% rate would probably both indicate code quality problems.

Code Quality Outputs

Two of the other “measures” that Oren refers to are actually the goals we’re trying to achieve:  ease of change over time and the quality of the delivered product.  Now, ultimately these are all we actually care about.  Code quality itself is merely a means to an end: getting things done.  But again, we have two problems: there’s no actual measures here, and we’re not actually looking at the code quality itself.  Let’s remind ourselves of Martin Fowler’s dictum:  you can’t measure productivity.  Now, with an open source project, we have some advantages over the classic corporate development scenario: mostly that people are less likely to game the system.  However, there are ways of achieving these aims that don’t involve improving the quality of the code base: I’ve improve the project velocity and project quality of legacy systems by spending a lot of time developing test suites for them.

Both of them suffer from the serious question “relative to what?”.  It’s taken NHibernate 2.1 two years to hit GA.  Does that mean the code quality is bad?  No, probably not.  There are any number of possible reasons for this: the changes were large and complex, the contributors couldn’t devote much time to it, sticking with 2.0 slowed things up, the project admininstrators didn’t regard going GA as a priority.  I’m sure you could think of more without much trouble.

You can’t measure project velocity “in general”, because it depends what you want to do to the codebase.  The release notes testify to a phenomenal amount of number of improvements, but the criteria API still isn’t LINQ-complete.   Why is this?  Well, it’s because the criteria API was developed well before LINQ was on anyone’s radar.  With the best will in the world, sometimes you end up with the wrong abstraction.

Reported Bugs

Now here, we have something we can measure.  I’ve already spoken about the dangers of doing so, so take that as read.  The problem is, a quick look at JIRA doesn’t really seem to tell a good story.  More than 10% of all logged issues are open.  The vast majority of those are unassigned.

Some of these are really arguable.  The two most popular open issues are a problem with Oracle stored procedures and a desire for the lazy loading of columns.  Both can be filed under the category “working with legacy databases”.  Do I care?  No.  Do the people voting on the issues care?  I’d say yes.  Have they submitted a patch?  No.  But then, submitting a patch would require you to be up to speed with the code base, which is where this whole discussion began.

So what is NHibernate’s Code Quality, then?

Frankly, I don’t know.  A naive scan of the code reveals a code base structured according to good engineering practice, but that won’t reveal indirect and subtle dependencies.  Ayende isn’t worried about the direct dependencies that do appear in the system, but it’s quite rare to meet a maintainer worried about this before he’s advocating a wholesale rewrite.

NHibernate, as a live port of a Java project, is special.  Code quality, as with everything else, is context sensitive.  However, whereas I’m not convinced Patrick’s metrics necessarily tell the story it initially appears, I’m still not convinced NHibernate has any measures of quality.  That’s not the end of the world: you don’t necessarily need a process for something that isn’t broken.  I’d keep an eye on JIRA, though.

Next, I want to talk about what readability and maintainability actually are.  And I don’t want to talk about metrics.