RailsConf Recap: Skinny Controllers

RailsConf 2007 was a blast. Koz and I were able to do a live “Rails Way” presentation in front of all 1,600 attendees, and it was a lot of fun. We’d like to thank everyone who helped (with their questions) to make it a success, and especially Chad Fowler (for inviting us to do a plenary session instead of a regular session) and O’Reilly and Ruby Central for all their work behind the scenes to bring the conference off so spectacularly.

Now that RailsConf is past, though, we’re looking forward to bringing The Rails Way out of hibernation. The next few articles will review the points we mentioned in our RailsConf presentation.

For this first article, I’ll be examining one of the controllers in an expense tracking application submitted by Brian Cooke. In general, the application was really well done, but I did find one example of a fat controller.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15

class ReportsController < ApplicationController
  def create
    report = params[:report]
    category_condition = report[:category] == "all" ? "" : "AND category_id = #{report[:category]}" 
    from = Date.parse("#{report["from(1i)"]}-#{report["from(2i)"]}-#{report["from(3i)"]}")
    to = Date.parse("#{report["to(1i)"]}-#{report["to(2i)"]}-#{report["to(3i)"]}").to_time+1.day-1
    @expenses = current_user.expenses.find(:all, 
                                           :conditions => ["created_at >= ? AND created_at <= ? #{category_condition}", from.to_s(:db), to.to_s(:db)], 
                                           :order => "created_at ASC")
    @graph = params[:report][:type] == "graph" 

    @total_price = 0.0
    @expenses.each {|exp| @total_price += exp.price}
  end
end

All that code obscures the purpose of the action. Looking at that, your brain has to do a lot of work to figure out what the intention is. To make the intention clearer, most of that code should be refactored into a model, but the question is: which model? You can get some idea of what model is needed by looking at the query parameter that is used: report.

Now, there is no “reports” table in the domain for this application. That does not mean there can’t be a Report model. Many people new to MVC who are learning Rails have the idea that all models correspond to database tables. The fact is that a model is just an encapsulation of an object, persistent or otherwise.

Consider the ReportsController#create action, rewritten to take advantage of a Report model:

1
2
3
4
5

class ReportsController < ApplicationController
  def create
    @report = Report.new(current_user, params[:report])
  end
end

Now that’s skinny. Looking at that action, it is now immediately obvious (at a high level) what it is doing. Skinny controllers are intention-revealing. Fat controllers are intention-obscuring.

Let’s take a quick look at how the Report model breaks down:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53

class Report
  attr_reader :user, :options

  def initialize(user, options)
    @user = user
    @options = options || {}
  end

  def graph?
    @options[:type] == "graph" 
  end

  def total_price
    @total_price ||= expenses.sum(&:price)
  end

  def expenses
    @expenses ||= user.expenses.find(:all,
      :conditions => conditions,
      :order => "created_at ASC")
  end

  def from
    @from ||= date_from_options(:from)
  end

  def to
    @to ||= date_from_options(:to)
  end

  private

    def date_from_options(which)
      part = Proc.new { |n| options["#{which}(#{n}i)"] }
      if part[1]
        Date.new(part[1].to_i, part[2].to_i, part[3].to_i)
      else
        Date.today
      end
    end

    def conditions
      conditions = ["created_at between ? AND ?"]
      parameters = [from.to_time, to.to_time.end_of_day]

      if options[:category] != "all" 
        conditions << "category_id = ?" 
        parameters << options[:category]
      end

      [conditions.join(" AND "), *parameters]
    end
end

Now, this is a lot more lines of code than the original action. That’s not a bad thing! More lines of code are always better, if they make the code easier to read and maintain. In the above, I broke the original code into discrete parts, each of which went into it’s own method. This has several nice side-effects:

You can now test the Report model in isolation, instead of having to write a functional test and assert_select your way through the output. The multiple instance variables in the original action are replaced now with getter attributes on the model, so in the view you have @report.total_price instead of the more ambiguous @total_price. You can now use the form_for helper to simplify the view.

1
2
3
4
5

<%= form_for(:report, @report, ...) do |f| %>
  From: <%= f.date_select(:from) %>
  To: <%= f.date_select(:to) %>
  <!-- etc.... -->
<% end %>

Thanks to Brian Cooke for the submission!