Rails: Cleaning up ugly controllers

Question

I'm all for the skinnier controller, fatter model mindset.

I wanted to know how to go about:

1. How you identify things that should be moved to your model (assuming you're like me and you get lazy and need to refactor your controllers because you just shove code in there)

2. How you write and structure the creation of new elements in your controller. See following example.

Example

I had a relatively messy controller for a polymophic "vote". I've cleaned it up pretty well, but I wanted to know if I could improve this action a little better:

def up
 vote = Vote.new
 vote.vote = true
 vote.voter = current_user    
 vote.voteable = Recipe.find params[:id]
 vote.save
end

To me it's just a little ugly, and I probably should just use create instead of new, but I'm wondering if I'm driving down a deadly path here by using a non-standard action (concerning REST).

I'm working on switching it to new right now. But I definitely wanted to get the point of view of the community about.

Answer 1

In RESTful controllers, especially with a lot of actions I sometimes create a before_filter to load the object:

before_filter :load_recipe, :only => %w(show edit update destroy up down)

private
def load_recipe
  @recipe = Recipe.find(params[:id])
end

In your case I might consider moving voting to the user model so you would have something like:

def up
  current_user.vote(@recipe)
end

And then in your model:

class User < ActiveRecord::Base
  has_many :votes

  def vote(object)
    votes.create(:vote => true, :voteable => object)
  end
end

The benefit of that is that you can easily test the behavior of voting in isolation, and it can be re-used if there are other places you might want to enable voting (voting as in implicit result of another action, voting via API, mass-voting, etc).

Answer 2

The key to this is Test-Driven Development. Once you make it a habit, the question of where to put code is answered for you 95% of the time. Here's why.

Unit testing (model testing in Rails) is the easiest place to test code. Model methods should be unit tested "black box" style - meaning you don't care what's inside the method, only that input X provides output Y. This will also cause you to write a greater number of smaller methods in your model, instead of very large methods. The easier it is to test, the better - and not just for testing's sake. Simpler methods are easier to override by other code, which is a big advantage of Ruby.

Controller (functional) tests, on the other hand, will find you caring more about what happens inside the action, since those methods aren't cut and dry input/output scenarios. Database calls happen, session variables are set, etc. Shoulda is a great test suite that automates a lot of this for you.

Finally, my advice is to look inside some of your favorite plugins to see how they're doing things. And if you're interested more in testing, I have an article about restful controller tests in Shoulda that might get you started.