---
title: 'Code Review: Ruby and Rails Idioms'
teaser:
tags: web,rails
author: Dan Croak
published_on: 2011-07-27
---

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](https://thoughtbot.com/blog/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](http://www.youtube.com/watch?v=70tF-9Uf-Mk).

## Rendering partials

Original:

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

Refactored:

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

The documentation for
[render](http://api.rubyonrails.org/v3.0.8/classes/ActionView/Rendering.html#method-i-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?](http://api.rubyonrails.org/v3.0.8/classes/ActiveRecord/FinderMethods.html#method-i-exists-3F)
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](http://api.rubyonrails.org/v3.0.8/classes/Object.html#method-i-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](http://avdi.org/devblog/2011/07/05/demeter-its-not-just-a-good-idea-its-the-law/).
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<abbr title="Structured Query
Language">SQL</abbr>. Use the documentation on
[ActiveRecord::ConnectionAdapters::DatabaseStatements](http://api.rubyonrails.org/v3.0.8/classes/ActiveRecord/ConnectionAdapters/DatabaseStatements.html)
as your guide.