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 straightSQL. Use the documentation on ActiveRecord::ConnectionAdapters::DatabaseStatements as your guide.
