Marty Andrews

artful code

Friday, May 29, 2009

Enforcing Ruby code quality

Over the last year or so, there's been lots of great tools coming out in the Ruby community that help with static analysis of code. We've now got flay, flog, reek and roodi. metric-fu runs all of these tools (plus more) to generate nice reports.

Generating nice reports is all well and good, but frankly it's not enough. We need to start enforcing code quality, not just reporting on it. The Java community is way ahead on this front, which I find rather embarrassing as a Ruby developer. Fear not though, I'm going to show you exactly how to set it up here.

Getting Started

First of all, you need a continuous integration build. Don't have one? Fail. Go have a good hard look at yourself in the mirror and consider if you really want to be a professional software developer or not.

If you're still here, you need the tools installed. That's pretty easy:

What you've just installed is a set of tools that check for duplicate code, complex code, code smells and code problems. The last one (metric-fu) will give you a report to look at if you need to browse for more details.

Next, you need to add some tasks to your rakefile. For my rails applications, I have this in lib/tasks/quality.rake, but you can put it anywhere that gets loaded by your build.

Now, add the quality task to the list of things done in your continuous integration build. That will make sure these things run all the time. Try running rake quality manually. It will almost certainly fail on either flog, flay or roodi. I'll show you how to tweak them one at a time.

This tells me that I have four methods in my code base that have a flog complexity greater than 40. If you look back at line 11 of quality.rake shown above, you'll see that this threshold is configurable. Set that number to something that is just higher than the most complex method in your application (in my case, I set it to 90) and run rake flog again. It should pass.

What you've just done is create a stop loss for complexity in your application. If anyone checks in a method that is more complex than the configured threshold, your build will fail. In other words, methods are not allowed to get any more complex than the most complex method in your application now.

Unless you're working on a green fields application, I'll bet that the threshold you set was higher than you want it be. That method that was the most complex in your application needs some refactoring. You've got somewhere to focus your refactoring efforts now though. Pick that method, refactor it, and ratchet down the threshold to the next most complex method. Now you're incrementally improving the quality in your app in terms of complexity.

This tells me that I have two pieces of code with a mass greater than 30 that have been duplicated. If you look back at line 25 of quality.rake shown above, you'll see that this threshold is configurable. Set that number to something that is just higher than the largest mass of duplicated code in your application. This can be a little confusing, because the tool reports a number equal to that mass multiplied by the number of times it appears. In my case, the reported mass of 60 is actually 30 (a mass of 30 found in 2 places). So I set the threshold to 30. Run rake flay again. It should pass.

What you've just done is create a stop loss for duplication in your application. If anyone checks in code that duplicates an existing mass of code larger than the configured threshold, your build will fail. In other words, people can't copy and paste code in a fashion any worse than they already have.

Again, that threshold you set is almost certainly higher than you want it be. That largest mass of code that was duplicated needs some refactoring to make your code more DRY. Find that code, refactor it, and ratchet down the threshold to the next largest mass of code. Now you're incrementally improving the quality in your app in terms of duplication.

Roodi

When I ran rake roodi for the first time, I got something like this:

~/Data/runway $ rake roodi
(in /Users/marty/Data/runway)
app/models/action_format.rb:10 - Method name "format" cyclomatic complexity is 12. It should be 10 or less.
app/models/action_parser.rb:11 - Method name "attributes" cyclomatic complexity is 12. It should be 10 or less.
app/models/action_parser.rb:30 - Method name "parse" cyclomatic complexity is 14. It should be 10 or less.
app/models/token.rb:173 - Method name "parse_incubation_date" cyclomatic complexity is 11. It should be 10 or less.
app/models/token.rb:226 - Method name "parse_incubation_days" cyclomatic complexity is 12. It should be 10 or less.
app/models/action_parser.rb:50 - Block cyclomatic complexity is 11. It should be 8 or less.
app/models/token.rb:11 - Block cyclomatic complexity is 10. It should be 8 or less.
app/models/action.rb:103 - Method "apply_defaults_from_name" has 23 lines. It should have 20 or less.
app/models/action_parser.rb:30 - Method "parse" has 23 lines. It should have 20 or less.
rake aborted!
Found 9 errors.

This tells me that I have several pieces of code failing Roodi checks. If you look back at line 34 of quality.rake shown above, you'll see a reference to roodi.yml. This is a config file that allows you to configure Roodi. Create that file in the root directory of your application and paste the following contents into it. It's what I have in my config file.

This config file describes a list of checks that get run by Roodi, and some parameters that are used to initialise those checks. Have a look at http://roodi.rubyforge.org/ for details on each of those checks. Make a call on which checks are appropriate for your project, and then change the configuration on each item so that your build just passes. What you've just done is create a stop loss for these coding problems in your application. If anyone checks in code that doesn't pass these configured checks, your build will fail.

It's possible that you've had to comment out some checks that you really want on, to get the build to pass. For the ones that are still turned on, you've probably had to increase some of the thresholds. You can find that code, refactor it, and turn checks back on or ratchet down the thresholds over time. Now you're incrementally improving the quality in your app in terms of coding checks.

Summary

The installation of metric-fu will give you some nice reports to browse and find the next worst problems in your application. Run rake metrics:all to get that report generated. Put some time aside regularly to clean up your code and ratchet down the thresholds as described above until you get them to a point you're happy with.

As far as I'm concerned, these tools are not an optional extra for your project. You must have them tools set up. It's unprofessional of you not to do everything you reasonably can to keep the quality of your code high. I hope this helps.

21 comments:

Great post, I didn't even know that there was this much tool support for quality analysis in Ruby.

One thing I never liked though was having to set my check levels at just one tick above the worst spot in my code. This means there is nothing stopping anyone from checking in code that is as bad as what you currently have. I like my style rules to describe my current idea of good quality code whether my existing code conforms or not.

One solution (if the tool or your own hand rolling supports it) is to set a violation threshold (accounting for your existing code) and then set your rules at the ideal levels. This stops anyone from checking in code that doesn't pass muster while you are cleaning up other areas of your codebase.

I blogged about this on the same day at http://fuglylogic.com/2009/05/29/keeping-on-the-tail-of-code-quality-with-a-ratchet/

You say "Most style checking tools like Checkstyle have the ability to set a maximum violations figure". Do you know of any others? How do you know which violations they are?

Don't get me wrong. I like the idea, but I think it's only half way towards the real solution, which should know exactly what fails, and automatically remove them from the acceptable violations list once they've been refactored.

When faced with a choice of not enforcing, or enforcing at a level just higher than my current code, I choose the latter.

You don't just use the number of violations; you need to combine this info with an actual violation report to get any use out of it.

The way we implemented this before with Checkstyle was to check the XML violations report into SVN. We then set the build up to use SVN diff to differentiate between which violations you had just introduced and which were already there.

Each run would effectively clobber the violations report file so that if you removed violations as part of the build it would also be removed from the versioned report. If there were no changes then SVN wouldn't detect a content change.

I'm thinking of blogging a more specific recipe to detail how we achieved this using Ant and Checkstyle (aint that pretty though).