Another topic we touched briefly on at RailsConf was the idea of named callbacks.

Consider this snippet (also from Brian Cooke’s expense tracking application):

1
2
3
4
5
6
7
8

class Expense < ActiveRecord::Base
  protected
    def before_create
      if self.created_at == Time.now.to_date.to_time
        self.created_at = Time.now
      end
    end
end

One thing to keep in mind here is that when a new Expense record is created, the created_at column is used to track when the expense originally occurred, not when the record was created. As a special case, if the timestamp is 00:00 of the current day, then it is assumed to actually be the current time.

Now, looking at that code, it’s definitely not immediately obvious what it is trying to do. In fact, it took me a few minutes of steady concentration (and cross-referencing other parts of the project) to understand it. The fact that it uses a generic “before_create” callback makes it hard to know the purpose of the method, and the use of “Time.now.to_date.to_time” (though effective) is pretty intention-obscuring.

Here’s a clearer, more self-documenting approach, using a named callback:

1
2
3
4
5
6
7
8
9
10

class Expense < ActiveRecord::Base
  before_create :make_created_now_if_created_today

  protected
    def make_created_now_if_created_today
      if self.created_at == Time.now.beginning_of_day
        self.created_at = Time.now
      end
    end
end

The named callback helps make it clearer what the purpose of the method is (though in this case, an additional comment would not be amiss). Also, ActiveSupport comes to the rescue, allowing us to convert the convoluted “Time.now.to_date.to_time” into the more self-documenting “Time.now.beginning_of_day”. (Alternatively, you might prefer “Time.now.midnight”, though I find “beginning_of_day” to be clearer, since it reveals the intention better.)

Always look for ways to make your code document itself. Ruby is one of the most readable programming languages I’ve ever used, and it’s a pity to not take advantage of that readability as often as you can.

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 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:

1. 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.
2. 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.
3. 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!