GIANT ROBOTS SMASHING INTO OTHER GIANT ROBOTS

Written by thoughtbot

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.