Good and Not So Good Rails Code

I've found I learn best by example. If you do too, these examples of good and not so good Rails code might be useful.

This post is targeted at newer Rails programmers. If you've moved beyond the beginner stages you might not find much here for you.

Onward!

Let's start with some code that's not so good.

# app/controllers/book_controller.rb
def new
  @book = Book.find(params[:id])
  ...
end

def create
  @book = Book.find(params[:id])
  ...
end

def edit
  @book = Book.find(params[:id])
  ...
end

Why it's not so good:

This controller contains repeated code. ALWAYS BE WARY OF THIS! Pay particular attention when you're doing the exact same thing in multiple places. This type of code should almost always be refactored. In Rails, this duplication can be removed using a before_filter.

Better code:

# app/controllers/books_controller.rb
before_filter :find_book

def new
  ...
end

def create
  ...
end

def edit
  ...
end

protected

def find_book
  @book = Book.find(params[:id])
end

Why it's better:

The repeated code has been pulled out into a method (find_book). Now, changes to the way we find books will be easy to make in this controller. Notice also that we made it protected. We only need to use this method in this controller, and a gentleman's objects share as little as possible.

# Return an array of titles

def all_titles(books)
  book_titles = []

  books.each do |book|
    book_titles << book.name
  end

  book_titles
end

Why it's not so good:

Local variables can make complicated code more clear, but the variable in this method isn't pulling its weight. It's just a box around the stuff we're really interested in.

Good code:

# Return an array of titles

def all_titles(books)
  books.inject([]) { |book_titles, book| book_titles << book.name }
end

Why it's better:

It is a single, clear line, where each token pulls its weight. Experienced Ruby users will find this version clearer, despite its greater density.

It is possible you find this version less clear. A few months ago, I would have felt the same. Back then, inject felt a little scary: I'd only bumped into it other peoples' code, and never written a fresh one myself. However, after a bit of experience, I find it the perfect tool in the certain situations. Think of using it particularly when you want to build up a collection (like an Array or Hash) from some other collection.

If you'd like to improve your skills with inject:
1. Check out Jay Field's inject post (good)
2. Read its documentation and experiment in irb (better)

Update after posting:
Several commenters have (rightly) pointed out that this code is better written thusly:

# Return an array of titles

def all_titles(books)
  books.map(&:name)
end

Quite right guys. Inject is still great in the right spots though, and it's well worth your time to learn it.

# app/views/books/index.html.erb

<% for book in @books %>
  <%= render :partial => 'shared/book', :locals => { :book => book }
<% end %>

Why it's not so good:

You're calling a partial on a collection of objects, passing each one in through a variable. This idiom appears in Rails so frequently that a better syntax was created for it.

Good code:

# app/views/books/index.html.erb

<%= render :partial => 'book', :collection => @books %>

Rails will render the partial once per item in @books. Each item will be available in the variable 'book' (this is the name of the partial, NOT the singularization of your collection's name).

Why it's good:

We're being efficient by leveraging functionality that Rails already provides for us. This version makes its intention clearer: "I'm rendering a collection." The goal of the first version is less obvious, and takes longer to parse.

Similar to your 'repeated code' alarm, you should have a 'roughly a million programmers must have written these exact lines in the past' alarm. When that one goes off, check the Rails API and browse some plugins. Rails is great about providing you efficient ways to perform common tasks.