Ruby Code Audit Checklist

Noel Rappin, Monday, July 7, 2008 @ 11:11 am

Over the last few weeks, I've found myself increasingly in the position of having to evaluate an existing Rails code base, either as part of the code audit service that we offer, or because we're being asked to take over an existing Rails application.

The goals of an audit like this vary somewhat but generally are a) determine the level of cluefulness of the previous coders, b) figure out how to run the program locally, and c) document the first two parts so that when we tell the client how complicated it's going to be we can offer some concrete description of why. Often there are specific features we need to add or bugs to fix, so there's a particular focus on those features.

This always reminds me a little bit of those home-flipping shows when the new buyer starts tearing out the drywall and discovers the lost colony of termites... Anyway, here's the checklist

Quick Check: Version and Plugins

The first thing I do is usually a quick glance at vendor/rails to check what version of Rails the app runs under. Not having a vendor/rails is a negative mark. I also check to see what plugins are installed, as that's usually a pretty good high-level guide to what the functionality of the application is -- ActiveMerchant implies e-commerce, for a simple example.

/test or /spec

The first actual code I check out is the unit tests -- it's the foundation of the app, and if the tests are poor or nonexistent that's an extremely worrisome sign. Nonexistent is easy enough to spot, if there are tests I try to run rcov (if the app doesn't have the rcov plugin, that's another negative sign.)..

Well, first I try to run the tests. Ideally, this is simple, make sure that I have a database.yml file (best practice is that the codebase has a template file for me to copy...), run db:create:all, run db:migrate, and run rake. Generally this is the point where I have to load some gems. Failing a migration from scratch would be a big problem. If the tests themselves don't run, that's also bad.

Having tests that are out of date with the application is almost worse than having no tests at all -- if the tests are failing, I'm going to have to fix that before any forward work can be done.

Checking the quality of passing tests is a little more difficult. Lack of coverage is a nicely objective metric. Very long test methods generally indicate to me that TDD was not followed in the creation of the app, and that the tests are likely to be brittle and not map very well to the actual logic points of the system. It's also common to see a lot of repetition in tests -- often developers don't spend as much time cleaning up test code. Again, that's sign that future maintenance is going to be difficult.

rake stats

Somewhere around here I usually fire up rake:stats just for the heck of it. I don't put a whole lot of stock in it -- it can be easy to fool. But a 1 - 0.2 test/code ratio is a red flag. (I think that RSpec boilerplate is pretty long, especially for controllers, which makes it possible for an app with relatively few actual custom tests to still score really well on that metric.

I also look at the ratio of controller to model code. Again, I don't take it too seriously, but I'd rather see more code in the model then not.

Run the App

At this point, I usually try to run the app. In theory, if the tests run, then the app should run. But, well... If there's a Capistrano script, that often has some hints about other dependencies or quirks. Data is also another problem -- some apps require some basic data in the database, often semi-static data like category names. If the code base doesn't have a setup for this in the code migration, sometimes it'll be in a rake task. Sometimes a snapshot of a real database will be made available, obviously that's helpful.

Most of the time, an app that I'm auditing is expected to run, and I'm not trying to do a full QA test on it. Mostly I'm trying to see what it does, and especially what it does that looks complicated to guide my search in the app code.

Application Code

Now it's time to search the app directory. The first part of this is just opening each file. This is the kind of thing you could almost do with the font size way down... I'm not so much trying to understand the details of the code as the shape of it. A lot of small classes, or fewer, larger classes? How long are the methods? A lot of long methods really scare me, especially in conjunction with minimal unit tests. More subjectively, does the layout look sloppy? Are there coding constructs that indicate a Ruby-specific style, or does everything look like translated Java? Is metaprogramming used where appropriate? Is there code in the controllers that should be elsewhere? Are the views too knowledgeable about the rest of the system?

Through that quick glance I identify the most interesting bits of code, and then I go back to them in more detail, asking questions like: Can I understand what this is doing? Is this the best way of doing it? Is there an obvious time or space inefficiency? Does the design make sense?

Configuration

The config directory is usually my last standard stop. Destinations here include the Capistrano deploy script, it it's there, any other configurations of interest (like an Amazon S3 setup). The environment.rb file is another good place to look for configurations, and you can't overlook config/initializers -- it's a good place to hide dependencies and monkey patches that you might overlook.

The routes.rb file will give some ideas about the site in general -- it's the Rosetta stone between URL and controller, and it's common to have to dig here to figure out what's going on if the routing is at all complicated. It's easy for this file to get out of control if nobody is paying attention, so it can tell a lot about the general nature of the programming team. A lot of additional methods added to a RESTful controller, for example, which might not be the best design solution.

Related posts:

1. Ruby/Rails: Loading Seed Data for Tests
2. A Java Programmer’s transition to Ruby on Rails
3. Resolved: Should schema.rb be included in your source control?
4. Rails Testing Frequently Asked Questions — The Non-Code Version
5. Back To The Ruby Mines

Comments (3)

Software Development

Comments: 3 so far

• Leave a comment
• Comment feed for this post
• TrackBack URL

1. 

   Failing a migration from scratch would be a big problem.

   It really shouldn’t be – that’s what the schema.rb is for.

   The “best practice” (I hate that phrase!) is to use db:schema:load when you first setup a new project, which is not only a lot quicker then running all the migrations, but is also less likely to explode later on because something changed.

   You also mentioned in your project tutorial that you don’t keep schema.rb in version control. That’s possibly one of the most important files you can have there, since it’s used to bootstrap a new production server.

   Comment by Jon Wood, Thursday, July 10, 2008 @ 4:40 am

2. [...] a couple of weeks ago in the code audit post I wrote that I would consider it a big problem if the code base I was investigating couldn’t run [...]

   Pingback by Pathfinder Development » Resolved: Should schema.rb be included in your source control?, Friday, July 18, 2008 @ 12:39 pm

3. [...] A couple of months ago, I wrote about auditing existing Rails applications en route to potentially taking them over. [...]

   Pingback by Pathfinder Development » Dealing With A Legacy, Friday, September 5, 2008 @ 2:03 pm

Leave a comment