You Are What You Measure
Listen to this article
James mentioned that one of his colleagues had been compiling stats on various projects and how they rated in terms of Cyclomatic Complexity (A.K.A. McCabe's Complexity). Quite rightly, everyone on the team was ecstatic to find that the project ranked top of the list (ie. lowest average complexity) in a field of various open and closed source projects.
Tools such as Checkstyle and PMD (to name but a few) make collecting these kinds of statistics very easy. What's more, incorporating these tools into your developer and continuous integeration builds makes enforcing limits on whatever metrics you like almost trivial. And that is exactly what we had done.
Comprehension and testability of code vary with complexity. Overly complex code is difficult to maintain, difficult to test and more likely to contain bugs (hidden flaws). Experience on previous projects (rather than just theory) had taught us that a low value for cyclomatic and it's cousin NPath complexity was indeed a good predicter of problematic code. On these projects, we had always cranked the threshold right the way down and had found this to have a positive influence on the code base.
In true Pavlovian style, and with nothing but ultruistic intentions, we decided to make cyclmatic complexity (along with dozens of others) a constraint on this project as well. It obviously follows then that our project would place at or near the top of the list. We had made it a constraint, enforced it in our build and therefore the only possible outcome was a low average.
But as always, the devil's in the detail. It soon became apparent that like so many other checks, it is possible to subvert the process. The worst part is that few developers do so out of malice. More often than not a check will fail and the developer will (often quite ingeniously I might add) code their way around it. Without knowing any better, it is quite easy to inadvertently increae actual complexity whilst adhering to simple metrics.
I have a love-hate relationship with development tools. In the right hands they are invaluable but you can't just give inexperienced developers tools and expect them to do as well. Tools + Monkeys != success. In fact as we discovered, left unchecked the opposite is much more likely to occur. Luckily for us we had a much better ratio of experienced/inexperienced developers so we caught most of the problems early and entered into intense education (ala Clockwork Orange - just kidding). I think it's safe to say that in the end, the benefits certainly outweighed the potential dangers.
While certainly interesting, relying on a single metric can be very dangerous. Blind adherence to any number of metrics can disatrous. In general it's often far safer to let tools do the grunt work to find the smells and even make recommendations, then employ the expertise of more experienced developers to deoderise. It's also a good idea to use several metrics in combination that address different, sometimes competing, concerns.
Comments
I've had a chat with this about a couple of cow-orkers, and imho, it's all about "goal alignment".
If you put in a goal that says "test coverage 90%", "cyclomatic complexity max of 10", "6 story cards per iteration per person", then developers will do a good job of achieving it.
Of course, be careful of what you wish for, you just might get it ;-). I'd rather set the goal of "finishing the project alive, to the satisfaction of the customer and the pleasure of the team".
Personally, I like to use those sorts of metrics as a "health check". If I suspect some sort of smell, then I like to run those to see what's going on and give me some targets to shoot at.
Posted by: Jon Eaves | September 8, 2004 09:17 PM
Agreed. BTW, how does one Ork a Cow? :P
Posted by: Simon Harris | September 8, 2004 09:34 PM
I think a lot comes down to the quality of people you can get. Tools like Checkstyle work really well at helping experienced developers avoid smells, by pointing them out. They can then become a constraint on junior developers who don't really understand the point of it.
Take the Cyclomatic Complexity measure. In Checkstyle, that's an easy one to get around by extracting the body of the if/for/whatever to another method. But has this _really_ made the code simpler? Of course not. However, if you view the Checkstyle alert as a warning rather than a whip, you can take the time to consider if there is a better way. And there nearly always is. Similarly, I use a method-size constraint. When I hit a method size limit, this alerts me that I can probably find a better way of expressing the method. OTH, I've seen other people literally just chop the method in half (and call the new methods MethodNamePartOne and MethodNamePartTwo!) just to get around the restriction.
I'm yet to see it with Simian, but it's only a matter of time, I'm sure.
Still, all things considered: the code I see with these constraints is still better than what I tend to see without these constraints. Not as good as it would be if people followed the intent of the constraint instead of the letter, but still...
OTH, my preferred constraint would be to assign one decent experienced developer for every junior. Then I could happily throw the constraints away. Well, I can dream...
Posted by: Robert Watkins | September 8, 2004 10:01 PM
Agreed again. We introduced CheckStyle recently on a few projects, and I was somewhat less than delighted when I looked at some of the code that was subsequently written and found it littered with 'private final static int FOURTEEN=14' type declarations to get around the 'no magic numbers' rule.
Posted by: Chris May | September 8, 2004 11:17 PM
Speaking of checkstyle, I found this little gem in our tests: a constant for zero (although AFAIK 0,1 and 2 are not magic numbers). When checking a list was empty, it checked that its size equaled ZERO_NUMBER. People are very inventive..
Also, after some refactoring, some code compared something with 1+2, and checkstyle was silent..
I guess the only solution is no code ownership and very often code reviews.
Posted by: Radu | September 9, 2004 01:43 AM
I think that's Jons view that checkstyle should be used as a code review tool not as a blunt instrument.
Posted by: Simon Harris | September 9, 2004 01:58 AM
I agree with Jon in principle. In practice, however, I find that less experienced people benefit from the constraints regardless of how they try to jump around and avoid them.
In order to use Checkstyle/Simian/PMD/FindBugs/etc as a review tool, you can't be inundated with errors when you run them. Realistically, having a blanket "Break The Build" rule is about the only way to ensure that there aren't a million errors.
I personally find the need to use Checkstyle, etal as a blunt instrument distasteful. I'm also convinced that in my environment, at least, with our 10-1 junior-senior developer ratio, that it is essential. I find that ratio even more distasteful than the need to run Checkstyle.
Posted by: Robert Watkins | September 9, 2004 07:21 AM
Having been the creator of Checkstyle, is really does make be cringe to see the hacks people do to pass Checkstyle. The whole motive of Checkstyle was to be an aid in preparation for a code review. Nothing replaces the value of another pair of eyes reviewing your code.
Posted by: Oliver Burn | September 9, 2004 10:45 AM
Oliver, it's said that one of the signs of a good tool is that people will put it to uses the creator never imagined. Nowhere is it said that the creator would have to _like_ those uses... ;)
Posted by: Robert Watkins | September 9, 2004 10:52 PM
Checkstyle is great for those sometimes-necessary slaps on the hand for careless mistakes. I just wish there was some sort of tool that can take an existing codebase that fails the (quite minimal) checkstyle checks in about 20000 places and makes it lovely. Maybe then the developers on my team wouldn't ignore those very helpful icons in WSAD (thanks to the Eclipse plugin) and would stop doing things like "if (someboolean == true) sometotherbool = true;" and making methods with cyclomatic complexity >25 etc! aaaargh!
I wonder if that would be possible...?? :D
Posted by: Dutch | September 10, 2004 03:57 AM
Dutch are you looking for FIXSTYLE? I've heard that cry for help before. Many IDEs (eg Eclipse and IntelliJ) have good refactorings in them that can fix the most obvious problems. A lot of other Checkstyle checks can be fixed with a simple housestyle-compliant code formatter. Tricky issues like Cyclomatic Complexity tend to point to deeper problems though (ie. of Design).
My only complaint about Checkstyle is that neither of the extant Eclipse plugins offer a really elegant interface into it (both have flaws), and once developers discover an Eclipse plugin for something they stop running the Ant task (we had to work a lot to wean developers off the flawed plugins, which stopped some checkstyle errors being indicated inside the Eclipse environment). I know I should fix this problem myself by writing or improving the plugins instead of just bitching about it, one day I might even jumpstart my lazy arse into action and write a plugin myself. One day.
Posted by: Scot Mcphee | September 13, 2004 11:54 AM