RailsConf Recap: Skinny Controllers

Posted by Koz Friday, June 01, 2007 04:12:00 GMT

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!

Comments

Leave a response

  1. Luis Lavena – June 01, 2007 @ 05:10 AM

    Jamis,

    What a excellent example!

    When I return to some code I wrote 6 months ago, I feel a terrible headache trying to figure out what I wanted to do there

    Often, as REST shows up, resources didn’t map directly to models, and sometimes, models don’t need to be persisted, they just serve for linking information across multiple resources.

    Bookmarked this article ;-)

    Regards,

    L.

  2. Max – June 01, 2007 @ 11:29 AM

    Wow, great article. I do not have to look at one of my app’s code in order to know that there are multiple occurrences of exactly the same kind of mixed up controller code. Thank you very much and keep up the good work.

  3. Neeraj Kumar – June 01, 2007 @ 12:28 PM

    I hopped over to gotapi.com and looked for ‘which’ command. No result. So what is which command? I am referencing line number 33 in the above example.

  4. Jim – June 01, 2007 @ 01:28 PM

    Been following TRW since it started and I have to say it’s probably been the most helpful to me in understanding some key concepts in my switch over to RoR. This article stands out amongst the best and is exactly what I needed today. Off to refactor, just wanted to say thanks and keep up the effort. Glad to see TRW out of hibernation.

  5. Ben – June 01, 2007 @ 01:32 PM

    @Neeraj Kumar

    “which” was an argument to the date_from_options method.

  6. Brian – June 01, 2007 @ 02:47 PM

    Thanks for following up your RC2007 presentation with this excellent write-up! I attended RC2007 and was hoping for more ‘concrete’ examples like this (I garnered more ‘how-to’ information from the keynotes than from the sessions). It makes me realize how fat (or obese, or beached-whale-like) my controllers are AND shows me the light on how to go about improving them.

  7. Paul Hoehne – June 01, 2007 @ 04:27 PM

    As an application of Dave’s “question everything” talk, I’m going to put forward the contra-opinion. Part of this is comes from going through the same exercise (and similar evolution of thought) with struts, session beans, and entity beans in the J2EE fiasco.. err.. framework.

    As a general principle that’s nice, but I have a contra-example. I tried to break invoice selection methods (all_invoices, paid_between_dates, etc.) into an invoices model object from the invoices_controller. However, I quickly ran into a problem – pagination. Pagination is a method on the controller but not on the model. Would passing the controller back into the model method work so I could put all the logic back in the model? It might but it should make you feel uneasy to do so. So, I broke them into private methods on the controller, which return the pagination information and greatly simplified the controller method that is actually invoked to list invoices.

    If you have a better approach to handle issue like pagination, I would be very interested. (Some seem to eschew pagination but I could have 1000’s of invoices so some way to cut them into manageable sections for display – if not raw database traffic – is key).

  8. Jamis Buck – June 01, 2007 @ 05:19 PM

    Paul, as with anything, YMMV. However, I don’t see any reason why you couldn’t make your model pagination-friendly. That doesn’t mean the model needs to know about pagination, but there is nothing wrong with putting a method on your model such that it will return a specific “page” of data, something like “account.invoices.page(5)”. After all, all that is doing is returning a slice of the data, which is essentially view-independent.

    Perhaps I’m misunderstanding your counter-example. If so, please feel free to send us a more detailed example. Maybe we’ll even post a review of it!

  9. Trevor Squires – June 01, 2007 @ 08:16 PM

    Paul – I’ve found Bruce Williams’ paginator gem an excellent tool for this very thing – you can paginate anything with it – no need to tie paginator to a controller or active_record object.

    http://rubyforge.org/projects/paginator/

    HTH, Trevor

  10. Luis Lavena – June 01, 2007 @ 09:40 PM

    To answer Paul’s question about pagination (model pagination friendly).

    Making your model methods return a slice (maybe through options = {})

    A simple example:

    
# model
class Invoices < ActiveRecord::Base
  class << self
    def all_invoices(options = {})
      find(:all, options.merge(:conditions => [...], :order => 'created_at DESC'))
    end
  end
end

# controller (using Bruce Williams’ paginator gem)
  ...
  @pager_count = #grab here the count of items to paginate
  @pager = ::Paginator.new(@pager_count, 10) do |offset, per_page|
    Invoices.all_invoices(:offset => offset, :limit => per_page)
  end
  ...

    So far, it worked for us.

  11. Colin Bartlett – June 02, 2007 @ 01:18 AM

    Excellent post guys. Super follow up to on of my favorite talks from RailsConf. Thanks!

  12. barry – June 02, 2007 @ 01:37 PM

    just an “administrative” comment:

    I read your article and decided to subscribe to your RSS feed. I use Google Reader. Your code snippets do not display there. For instance, for the first snippet, all I see is:

    1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 class ReportsController < ApplicationController

    Can you help make these snippets viewable in Google Reader—or is it impossible?

    Thanks for any feedback.

  13. Jamis – June 03, 2007 @ 01:08 AM

    @barry, the code snippets show up fine in NetNewsWire Lite (sans highlighting, but that’s a CSS issue). Generally, though, if you want to see the code snippets, you’ll have to visit the site.

  14. barry – June 03, 2007 @ 07:07 AM

    Thanks for the feedback, Jamis. I’m going to contact the Google Reader team about this. But in the meantime, I guess I will be coming back to your site frequently. :-)

  15. Conrad – June 03, 2007 @ 11:45 PM

    Hi Jamis and Michael, I have been following your blog entries with great interest and I was wondering, are you considering doing a book in the future on this very topic? I feel that a book on RoR refactoring would benefit the community. I would even assist to make it happen. Please let me know your thoughts and I understand that you both are busy professionals. Well, keep up the AWESOME work with the site and I truly enjoyed the presentation that you two gave at RailsConf 2007.

  16. Xavier – June 04, 2007 @ 03:59 AM

    Jay Fields, under the guise of the “Presenter” pattern, has has some very handy blog posts on this topic (http://blog.jayfields.com/search/label/presenter). I have found his Validatable project (validations for non-AR models: http://validatable.rubyforge.org) especially handy in putting this abstraction into our app.

  17. Jerry Adams – June 05, 2007 @ 09:11 AM

    A beginner’s question: How do I get another model to be aware of this model? Here’s what I did: In the models directory I created a mappable_item.rb file, which contains the class MappableItem. I’d like another model to create an Array of those, but I get the error undefined method `MappableItem’

    I’m sure it’s something quite simple, but I can’t guess it.

  18. Jerry Adams – June 05, 2007 @ 09:33 AM

    Oh. I had written

    item = new MappableItem

    instead, of course, of

    MappableItem.new

  19. Eleo – June 07, 2007 @ 12:14 AM

    Man my controllers are pretty fat. I will have to do some refactoring like this. My app always seems to be in a state of flux~. Oh well, this is a good tutorial anyway. Thanks!

  20. Bosko – June 11, 2007 @ 11:04 PM

    I often do this and often also need an ActiveRecord-like xml serialization of the class (to_xml). The hack I currently use is to include ActiveRecord::XmlSerialization; unfortunately I also need to def attribute_names; []; end and def self.inheritance_column; nil; end to make it behave. Know of a better way?

  21. Jamis – June 12, 2007 @ 07:03 PM

    The easiest way to add xml serialization to your class is to just package up all the info you want as a hash, and then to_xml on the hash:

    1
2
3
4
5
6
7
8
9
10

    class Report
  # ...
  def to_xml
     attrs = { :from => from,
               :to => to,
               :total_price => total_price,
               :expenses => expenses }
     attrs.to_xml(:root => "report")
  end
end

    You’ll find that even in ActiveRecord, the xml serialization mostly just boils down to Hash#to_xml.

  22. David – June 29, 2007 @ 11:10 AM

    Jamis, I think you may have missed the extra time added to the “to” field in the first version of the application – you don’t seem to do anything different in your “def to … end” block than your “def from … end” block. But otherwise, great example! Thanks.

  23. Jamis – June 29, 2007 @ 02:32 PM

    @David, actually, it’s still there, just refactored. Look in the conditions method; you’ll see “to.to_time.end_of_day”. It’s the same effect. The reason for leaving the “to” method itself alone was so that you could use it to recover the value that was originally input (so that, for instance, you could use the form helpers in the view).

  24. Eleo – July 05, 2007 @ 04:37 AM

    I just realized how ridiculously massive some of my actions are. Well I just shortened them to a few lines of code. It does take a different sort of mindset to put code you previously thought was proper in a controller, in a model, but in hindsight it is not that difficult at all.

  25. Apostlion – January 08, 2009 @ 10:50 AM

    You’ve been Ruby Reddited, congrats on that.

    Red and enjoyed this article back when it was published… What can I say, with a tendency to build mammoth controllers, I need some more coder discipline to spew the code I want.

Comment