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:

sudo gem install flay
sudo gem install flog
sudo gem install reek
sudo gem install roodi
sudo gem sources -a http://gems.github.com
sudo gem install jscruggs-metric_fu

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.

    1 require 'flog'
    2 require 'flay'
    3 require 'roodi'
    4 require 'roodi_task'
    5 require 'metric_fu'
    6 
    7 desc "Analyze for code complexity"
    8 task :flog do
    9   flog = Flog.new
   10   flog.flog_files ['app']
   11   threshold = 40
   12 
   13   bad_methods = flog.totals.select do |name, score|
   14     score > threshold
   15   end
   16   bad_methods.sort { |a,b| a[1] <=> b[1] }.each do |name, score|
   17     puts "%8.1f: %s" % [score, name]
   18   end
   19   
   20   raise "#{bad_methods.size} methods have a flog complexity > #{threshold}" unless bad_methods.empty?
   21 end
   22 
   23 desc "Analyze for code duplication"
   24 task :flay do
   25   threshold = 25
   26   flay = Flay.new({:fuzzy => false, :verbose => false, :mass => threshold})
   27   flay.process(*Flay.expand_dirs_to_files(['app']))
   28 
   29   flay.report
   30 
   31   raise "#{flay.masses.size} chunks of code have a duplicate mass > #{threshold}" unless flay.masses.empty?
   32 end
   33 
   34 RoodiTask.new 'roodi', ['app/**/*.rb'], 'roodi.yml'
   35 
   36 task :quality => [:flog, :flay, :roodi, 'metrics:all']

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.

Flog

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

~/Data/runway $ rake flog
(in /Users/marty/Data/runway)
    42.3: Token#parse_incubation_days
    60.4: Token#parse_incubation_date
    81.7: ActionParser#parse
    89.7: ActionFormat#format
rake aborted!
4 methods have a flog complexity > 40

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.

Flay

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

~/Data/runway $ rake flay
(in /Users/marty/Data/runway)
Total score (lower is better) = 164

1) Similar code found in :defn (mass = 60)
  app/controllers/signups_controller.rb:5
  app/controllers/user_sessions_controller.rb:6

2) Similar code found in :if (mass = 54)
  app/controllers/forgot_passwords_controller.rb:15
  app/controllers/signups_controller.rb:16

3) Similar code found in :defn (mass = 50)
  app/helpers/actions_helper.rb:43
  app/helpers/actions_helper.rb:49
rake aborted!
3 chunks of code have a duplicate mass > 25

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.

# AssignmentInConditionalCheck:    { }
# CaseMissingElseCheck:            { }
ClassLineCountCheck:             { line_count: 300 }
ClassNameCheck:                  { pattern: !ruby/regexp /^[A-Z][a-zA-Z0-9]*$/ }
# ClassVariableCheck:              { }
# TODO Get block cyclomatic complexity back down to 6 or less
CyclomaticComplexityBlockCheck:  { complexity: 12 }
# TODO Get method cyclomatic complexity back down to 8 or less
CyclomaticComplexityMethodCheck: { complexity: 14 }
EmptyRescueBodyCheck:            { }
ForLoopCheck:                    { }
# TODO Get method line count back down to 20 or less
MethodLineCountCheck:            { line_count: 27 }
MethodNameCheck:                 { pattern: !ruby/regexp /^[_a-z<>=\[|+-\/\*`]+[_a-z0-9_<>=~@\[\]]*[=!\?]?$/ }
ModuleLineCountCheck:            { line_count: 300 }
ModuleNameCheck:                 { pattern: !ruby/regexp /^[A-Z][a-zA-Z0-9]*$/ }
ParameterNumberCheck:            { parameter_count: 5 }

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.

8 comments:

  1. 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/

    ReplyDelete
  2. 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.

    ReplyDelete
  3. PMD has a similar feature (max rule violations).

    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).

    ReplyDelete
  4. Great post! You just inspired me to make a simple continuous integration app for RubyOnRails, at http://github.com/felipegiotto/Inotegration

    Hope you don't mind! I've taken your ideas, but I put your name on the credits! :D If you're interested, download the code at Github and send some feedback!

    Best regards,

    Felipe Giotto.

    ReplyDelete
  5. No problems at all Felipe. Good luck with your tool!

    ReplyDelete
  6. Regarding dupplication, I wrote a gem which generate reports (HTML, Textmate, Netbeans, etc …) of identical duplicate lines, backed by java tool Simian.

    http://wiki.github.com/garnierjm/dont_repeat_yourself

    ReplyDelete
  7. Thanks Jean-Michel. Simon Harris, who wrote Simian, happens to work with me at Cogent :) I've let him know about your gem.

    ReplyDelete