Code review: Ruby and Rails idioms

We often augment an existing Rails team or help build a Rails team on behalf of our clients. In the process, we do feature branch code reviews. Among other reasons, we do them to get everyone comfortable with Ruby and Rails idioms.

The following is a list of real code we’ve refactored (variable names changed to protect the innocent). This is mostly beginner-level stuff so please hold off on value judgments if you’re experienced.

Rendering partials

Original:

render :partial => 'admin/shared/errors',
       :locals => { :errors => @user.errors }

Refactored:

render 'admin/shared/errors', :errors => @user.errors

The documentation for render says:

If no options hash is passed or :update specified, the default is to render a partial and use the second parameter as the locals hash.

Determining a record’s existence

Original:

existing_song = Song.where(:user_id => user_id,
                           :album_id => album_id).first
if existing_song

Refactored:

if Song.exists?(:user_id => user_id, :album_id => album_id)

The documentation for exists? says:

Returns true if a record exists in the table that matches the id or conditions given, or false otherwise.

Conditionals when object is nil

Original:

<%= event.end_date.nil? ? '' : event.end_date.to_s(:long) %>

Refactored:

<%= event.end_date.try(:to_s, :long) %>

The documentation for try says:

Invokes the method identified by the symbol method, passing it any arguments and/or the block specified, just like Ruby Object#send. Unlike that method, nil will be returned if the receiving object is a nil object or NilClass.

It’s usually worth spending time identifying why you have to do this at all. If you can remove the scenario where the object is nil, that’s preferable.

Conditional assignment when object is nil

Original:

if !town
  town = user.town
end

Refactored:

town ||= user.town

Read it something like:

town or assign town

Data migrations

Original, in a migration file:

group_ids.each_with_index do |id, position|
  group = Group.find(id)
  if group
    group.position = position
    group.save!
  end
end

Refactored:

group_ids.each_with_index do |id, position|
  update "update groups set position = #{position} where id = #{id}"
end

By using ActiveRecord methods, particularly #save!, the original version increases the risk that the migration won’t run in the future.

If a validation is added later that causes the data for a future developer or CI box or performance/staging environment to fail validation, the migration will raise an error when all we wanted to do was set some initial positions.

The solution is to use straight SQL. Use the documentation on ActiveRecord::ConnectionAdapters::DatabaseStatements as your guide.

Sweep unused code into the dustbin with rcov

One of the oldest tools in the Ruby testing utility belt is rcov. Most developers know that they can use rcov to find parts of their application code that tests aren’t exercising at all. However, playing with rcov’s options a bit can help you clean up your application after refactoring or removing features. These techniques all rely on a well-tested application, so make sure you start with 100% C0 coverage.

Aggregate coverage

If you’re like us, you’re using Cucumber to test drive your applications from the customer’s perspective, and then filling in the gaps with RSpec coverage for edge cases. We’ve grown comfortable with the idea of aggregate C0 coverage between Cucumber and RSpec, with neither tool resulting in 100% coverage by itself. Splitting up coverage like this makes a tool like RCov even more important, but it’s a little trickier to get this working.

RCov supports an --aggregate option that will look at coverage shared between several test runs, regardless of the tool used for testing. If you’re using RSpec, you’ll want a task like this:

RSpec::Core::RakeTask.new(:rspec_aggregate) do |task|
  task.pattern = 'spec/**/*_spec.rb'
  task.rspec_opts = "--format progress"
  task.rcov = true
  task.rcov_opts = "--exclude osx\/objc,spec,gems\/ " +
                   "--rails --aggregate tmp/coverage.data"
end

You can get Cucumber coverage as well with this guy:

Cucumber::Rake::Task.new(:cucumber_aggregate) do |task|
  task.rcov = true
  task.rcov_opts = "--exclude osx\/objc,gems\/,spec\/,features\/ " +
                   "--rails --aggregate tmp/coverage.data -o 'coverage'"
end

In case anything goes wrong, you’ll want to clean up coverage in between runs:

task :clean_aggregate do
  rm "tmp/coverage.data" if File.exist?("tmp/coverage.data")
end

You probably want a task to run these all together:

desc "Run aggregate coverage from rspec and cucumber"
task :rcov => ["rcov:clean_aggregate",
               "rcov:rspec_aggregate",
               "rcov:cucumber_aggregate"]

Now you can run rake rcov to get an HTML report of your combined RSpec and Cucumber coverage.

Finding unused application code

We hit all the “happy paths” with Cucumber, and we generally hit the failure UI as well. For example, we’ll test the successful submission of a form and one failed submission in Cucumber, and then we’ll test all the individual validations that cause the form to fail from RSpec. That approach hits most of the application code with Cucumber, and although just running every line doesn’t indicate decent coverage, it can give you an idea of which parts of the application code are actually in use. You can generate an HTML report of application code that isn’t reached by Cucumber using a task like this:

desc "Find potentially unused app code"
Cucumber::Rake::Task.new(:unused) do |task|
  task.rcov = true
  task.rcov_opts = "--exclude osx\/objc,spec,gems\/,features\/ " +
                   "--rails --only-uncovered"
end

This will generate a report for each file containing potentially unused application.

Here’s an example line from a generated coverage/index.html:

If you click on the file, you’ll see unused portions highlighted in red:

We rely on combined coverage from Cucumber and RSpec, but anything that isn’t used in Cucumber is worth checking out. In this example, we quickly found out that this method could be removed. Removing unused methods and classes is a cheap and effective way to keep your application code small and readable.

Finding unused Cucumber step definitions

Cucumber’s mapped step definitions are useful for rapid testing without considering the implementation up front. However, an evolving application quickly develops a set of step definitions that aren’t used, cluttering up the features directory and slowing things down. You can use rcov to quickly identify steps that aren’t in use.

desc "Find unused cucumber step definitions"
Cucumber::Rake::Task.new(:steps) do |task|
  task.rcov = true
  task.rcov_opts = "--exclude osx\/objc,spec,gems\/,app\/,lib\/" +
                   "--rails --only-uncovered"
end

Notice that we added the app and lib directories, to ignore application code that isn’t exercised by Cucumber, and removed the features directory, so that it complains about unused feature code.

If you open coverage/index.html and click around, you’ll find steps highlighted in red:

If something is in red, it’s guaranteed that your test suite isn’t using it. You may want to keep some steps around that are never used (such as debugging steps), but most can safely be removed. Doing this regularly can keep your step definitions under control.

Taking things further

These examples are all wrapped up in Gist so that you can see them together: https://gist.github.com/795435

If you have improvements or corrections, please fork away!

There are a few other useful options in rcov:

• --text-coverage - print coverage to the console. Useful for terminal-oriented developers or for continuous integration. If you’re running this on CI, you may want to also add --no-color, which will disable ASCII color codes in the output.
• --text-report - print a summary to the console.
• --only-uncovered - removes files from the report with 100% coverage. Particularly useful when printed to the console or on CI.

Do you use rcov for anything else besides ensuring C0 coverage? If you have a cool recipe using rcov, let us know.