Buckblog: Skinny Controller, Fat Model

Skinny Controller, Fat Model

18 October 2006 — The "Fat Controller" anti-pattern is shown and dissected, and the reader is taken through the process of refactoring it into a more readable, maintainable, and testable solution — 5-minute read

When first getting started with Rails, it is tempting to shove lots of logic in the view. I’ll admit that I was guilty of writing more than one template like the following during my Rails novitiate:

<!-- app/views/people/index.rhtml -->
<% people = Person.find(
      :conditions => ["added_at > ? and deleted = ?", Time.now.utc, false],
      :order => "last_name, first_name") %>
<% people.reject { |p| p.address.nil? }.each do |person| %>
  <div id="person-<%= person.new_record? ? "new" : person.id %>">
    <span class="name">
      <%= person.last_name %>, <%= person.first_name %>
    </span>
    <span class="age">
      <%= (Date.today - person.birthdate) / 365 %>
    </span>
  </div>
<% end %>

Not only is the above difficult to read (just you try and find the HTML elements in it), it also completely bypasses the “C” in “MVC”. Consider the controller and model implementations that support that view:

# app/controllers/people_controller.rb
class PeopleController < ActionController::Base
end

# app/models/person.rb
class Person < ActiveRecord::Base
  has_one :address
end

Just look at that! Is it really any wonder that it is so tempting for novices to take this approach? They’ve got all their code in one place, and they don’t have to go switching between files to follow the logic of their program. Also, they can pretend that they haven’t actually written any Ruby code; I mean, look, it’s just the template, right?

For various reasons, though, this is a very, very bad idea. MVC has been successful for many reasons, and some of those reasons are “readability”, “maintainability”, “modularity”, and “separation of concerns”. You’d like your code to have those properties, right?

A better way is to move as much of the logic as possible into the controller. Seriously, isn’t that what the controller is for? It is supposed to mediate between the view and the model. Let’s make it earn its right to occupy a position in our source tree:

<!-- app/views/people/index.rhtml -->
<% @people.each do |person| %>
  <div id="person-<%= person.new_record? ? "new" : person.id %>">
    <span class="name">
      <%= person.last_name %>, <%= person.first_name %>
    </span>
    <span class="age">
      <%= (Date.today - person.birthdate) / 365 %>
    </span>
  </div>
<% end %>

# app/controllers/people_controller.rb
class PeopleController < ActionController::Base
  def index
    @people = Person.find(
      :conditions => ["added_at > ? and deleted = ?", Time.now.utc, false],
      :order => "last_name, first_name")
    @people = @people.reject { |p| p.address.nil? }
  end
end

Better! Definitely better. We dropped that big noisy chunk at the top of the template, and it’s more immediately obvious what the structure of the HTML file is. Also, you can see by reading the controller code roughly what kind of data is going to be displayed.

However, we can do better. There’s still a lot of noise in the view, mostly related to conditions and computations on the model objects. Let’s pull some of that into the model:

# app/models/person.rb
class Person < ActiveRecord::Base
  # ...

  def name
    "#{last_name}, #{first_name}"
  end

  def age
    (Date.today - person.birthdate) / 365
  end

  def pseudo_id
    new_record? ? "new" : id
  end
end

<!-- app/views/people/index.rhtml -->
<% @people.each do |person| %>
  <div id="person-<%= person.pseudo_id %>">
    <span class="name"><%= person.name %></span>
    <span class="age"><%= person.age %></span>
  </div>
<% end %>

Wow. Stunning, isn’t it? The template is reduced to almost pure HTML, with only a loop and some simple insertions sprinkled about. Note, though, that this is not just a cosmetic refactoring: by moving name, age and pseudo_id into the model, we’ve made it much easier to be consistent between our views, since any time we need to display a person’s name or age we can simply call those methods and have them computed identically every time. Even better, if we should change our minds and decide that (e.g.) age needs to be computed differently, there is now only one place in our code that needs to change.

However, there’s still a fair bit of noise in the controller. I mean, look at that index action. If you were new to the application, coming in to add a new feature or fix a bug, that’s a lot of line noise to parse just to figure out what is going on. If we abstract that code into the model, we can not only slim the controller down, but we can effectively document the operation we’re doing by naming the method in the model appropriately. Behold:

# app/models/person.rb
class Person < ActiveRecord::Base
  def self.find_recent
    people = find(
      :conditions => ["added_at > ? and deleted = ?", Time.now.utc, false],
      :order => "last_name, first_name")
    people.reject { |p| p.address.nil? }
  end

  # ...
end

# app/controllers/people_controller.rb
class PeopleController < ActionController::Base
  def index
    @people = Person.find_recent
  end
end

Voila! Looking at PeopleController#index, you can now see immediately what is going on. Furthermore, in the model, that query is now self-documenting, because we gave the method a descriptive name, find_recent. (If you wanted, you could even take this a step further and override the find method itself, as I described in Helping ActiveRecord finders help you. Then you could do something like Person.find(:recent) instead of Person.find_recent. There’s not a big advantage in that approach in this example, so it mostly depends on what you prefer, esthetically.)

Be aggressive! Try to keep your controller actions and views as slim as possible. A one-line action is a thing of wonder, as is a template that is mostly HTML. It is also much more maintainable than a view that is full of assignment statements and chained method calls.

Another (lesser) nice side-effect of lean controllers: it allows respond_to to stand out that much more, making it simple to see at a glace what the possible output types are:

# app/controllers/people_controller.rb
class PeopleController < ActionController::Base
  def index
    @people = Person.find_recent

    respond_to do |format|
      format.html
      format.xml { render :xml => @people.to_xml(:root => "people") }
      format.rss { render :action => "index.rxml" }
    end
  end
end

Give all this a try in your next project. Like adopting RESTful practices, it may take some time to wrap your mind around the refactoring process, especially if you’re still accustomed to throwing lots of logic in the view. Just be careful not to go too far; don’t go putting actual view logic in your model. If you find your model rendering templates or returning HTML or Javascript, you’ve refactored further than you should. In that case, you should make use of the helper modules that script/generate so kindly stubs out for you in app/helpers. Alternatively, you could look into using a presenter object.

Reader Comments

Good write-up! I’d argue that pseudo_id makes more sense like a helper method, just like dom_id in simply_helpful.

Christoffer Sawicki
18 Oct 2006

Good stuff. “Where should I put this code” is a question I quite often find myself asking when writing Rails code. Actually, it should get more attention in the Agile Rails book IMHO.

Geir
18 Oct 2006

Excellent! :P

I was also guilty of writing ugly code in my views before, as (almost) everyone has been, I’m sure. And I agree with Geir: the question of “Where does this go?” comes up often enough, and is one of the things that must be helped from early on.

One thing though, why would you filter by address.nil? in a find(:recent)? (I mean, what do addresses have to do with recent?) I’d leave the controller action to something like:

  def index
    @people = People.find(:recent).reject { |p| p.address.nil? }
  end

Nicolas Sanguinetti
18 Oct 2006

Excellent write-up in helping beginners understand the principles of Rails. Keep’m coming!

As Christoffer said, pseudo_id could be a helper, I think it fits fine in the model. I’m sometimes unsure whether to put little functions like that as part of the model or as a helper. Personally, I think helpers are better for things that are used beyond one particular model. However, I do find myself wondering whether I should put a helper that applies to a particular controller only in the controller file itself or in the helper file tied to the controller. I sometimes wonder whether the controller-specific helper files aren’t redundant, and that those helpers which are controller-specific should go in the controller, with those that are global in the application_helper file. Then you don’t have to look in both the controller and its helper file for stuff. (Same for models.)

zerohalo
18 Oct 2006

Wow! Thanks a ton. There’s a lot I can learn from this single post! Being new to Rails I often wonder if there is a better way to do something that I’m trying. This convinced me that most of the time there is a more Rails-y way.

I’d agree with Geir on the Agile book including more “refactoring.”

Rich
18 Oct 2006

Agreed! Unfortunately there are places where you cant have the clean templates with @db_variables initialized in the controller as one aspires to.

For example if you want fragment caching to actually save work you need to call down to the database from the view. Best I could make in such cases is placing the call to model method in a helper. But this just feels less intuitive. Am I missing something?

Zsombor
18 Oct 2006

Naturally, there are exceptions to every rule. The idea described in this article is something to aspire to, not to live religiously by. :)

That said, I’m not sure what you mean, Zsombor, about needing to call the DB from the view for fragment caching to work. Backpack uses fragment caching extensively and we don’t do any Model#find calls from the view there.

Jamis
18 Oct 2006

Very good explanation, that’s really usable!

Bart

Bart Braem
18 Oct 2006

Good article – definitely knowledge that needs to be spread wide and far. This stuff really makes your code better. Going to keep the link to this handy when helping some people learn Rails in the near future.

One trivial question relating to the address.nil point raised by Nicolas but from a different tack. Why is the nil address filtering being done in Ruby? Could that not be done within the SQL conditions so you avoid bringing back records from the database you are only going to reject anyway?

Goynang
18 Oct 2006

Yah, the whole address.nil? thing is very contrived. Sorry about that. It was only there to try and demonstrate “other” things that are commonly put in the view, which ought not to be. (sort, reject, select, etc.) With a little more thought, I could probably have come up with a better example.

Jamis
18 Oct 2006

Good stuff. Other benefits of this approach are…

- testing domain logic in models is simpler & clearer than testing it in controllers and views

- it makes your Rails console much more useful

Like the web user interface (with its controllers and views), both tests and the console, should be considered as important clients to the domain model.

James
18 Oct 2006

Jamis,

Another great example.

One thing, though: it doesn’t look like you’re escaping model data before you’re dropping into the HTML template. You might want to sprinkle some h() calls in there to guard against XSS attacks and broken HTML.

Cheers, Tom

Tom Moertel
18 Oct 2006

Good catch, Tom. Definitely. <%= h(person.name) %> is your friend.

Jamis
18 Oct 2006

Now to get rid of all those noisy <% and %>’s by switching to Markaby or HAML. ;)

Joe Ruby
18 Oct 2006

HAML certainly shows promise, but the last I checked it didn’t support iterating or a few other things that I feel are necessary in a template. Markaby, though…well, beauty is in the eye of the beholder, I suppose. :)

Jamis
18 Oct 2006

You don’t like Markaby? ;) (I do.)

Yeah, HAML not supporting iterating is a deal-killer for me.

Joe Ruby
18 Oct 2006

Could somebody please write the Find statement with the address conditions rolled into it.

Maybe this could be a final step in this tutorial?

Mr_mister
18 Oct 2006

Nice writeup!

Jon
18 Oct 2006

the system ate my comment :(

Anyways, the final view is just a view, but erb still doesn’t look that much like HTML. The same view could be rewritten in a DSL such as TAL which would take it from 53 non-alpahnumeric noise characters down to just 35, and from 9 different alphanumeric characters to just 5 different ones, only one of which is not in standard HTML.

Kevin Teague
18 Oct 2006

It’s so funny because something like this just came up on cocoa-dev.

Just from skimming this post, I think it’s basically the same issue. New Cocoa programmers are constantly trying to figure all sorts of logic out through the view. It’s often much simpler to add a few methods to the model objects, and talk to them (instead of the view) when you need state information.

It’s also amazing how many people want to “load data” into the views by force. In reality, the views pull data up through the data source, and the controller becomes much more passive.

Scott Stevenson
19 Oct 2006

I believe what Zsombor meant was that even if you cache your view output, the controller is still going to (redundantly) call Person.find_recent.

bodhi
19 Oct 2006

Nice write up. I’ve found I started to do exactly these sorts of things (moving more and more into the model where it’s appropriate). I love how adding a descriptive method to a model seems to change the entire dynamic of what you’re doing syntactically. It makes the rest of the code read so much better and is often very self documenting.

Cheers, John

johnpg
19 Oct 2006

Thanks Jamis. This is the best post on Rails I’ve ever read. No kidding.

Joe
19 Oct 2006

This all looks perfect, expect one thing. I don’t like the pseudo_id method, because it’s presentational.

Some day or another, you decide to make a link out of that “new”. Or maybe you want it in bold face, or maybe you want to give it an id to replace later as a result of an AJAX call.

Even without going that far, just consider internationalized UI (able to switch the language of the UI for each user individually). How do you know what language to give the word “new” in? That information is in a session probably, and models can’t (and shouldn’t ever desire to) access sessions.

So, well, pseudo_id’s place is among helpers.

Andrey Tarantsov
19 Oct 2006

Top notch

Bil Kleb
19 Oct 2006

Actually, I disagree that pseudo_id is presentational. It is no more presentational than id is; it simply handles the case where the record has not been saved (record.id returns nil if the record has never been saved successfully).

Jamis
19 Oct 2006

A tiny question: have you considered “render :partial, :collection…” to further clean the iteration in the view?

Piggybox
19 Oct 2006

Piggybox, yah, I thought about it, but within the context of this example, it wouldn’t have been any cleaner, and would actually have server the opposite purpose, further obfuscating things. In general, I recommend only using partials when a view is either complex enough that it needs to be broken down into parts, or when portions of a view need to be reused.

Jamis
19 Oct 2006

Another excellent writup! I think the model’s need to do more of the heavy lifting as I see a lot of code which brings in tons of records only to “reject” them later on when they could have been eliminated at the RDBMS side (I realize this was a made up example)

I think I am going to start putting a list of links to these wonderful articles together for my reference :)

cheers,

Amr

Amr Malik
20 Oct 2006

Backpack uses fragment caching extensively and we don’t do any Model#find calls from the view there.

So I do miss something! If Model#find calls are present in the view code either directly or indirectly trough helpers, then how can you benefit from fragment caching? There will be no database calls saved, and rendering html is usually fast leaving little to save …

What am I missing?

zsombor
23 Oct 2006

zsombor, I understand now what you are saying. The fragment caching we do only saves us the trouble of rerendering complex layout—we don’t actually avoid DB queries. (Well, we avoid some, since we lazy-load the associations on the page, but every request for a page will hit the DB for that page object.)

I still feel that hitting the DB from the view is nasty, but yah, I’m not sure how you’d work around the issue with fragment caching.

Jamis
23 Oct 2006

Thank you Jamis for following up, now it is much clearer.

zsombor
24 Oct 2006

I don’t understand how this line works:

new_record? ? “new” : id

Could someone please render that in English, or leave a link to an appropriate reference, please? It’s hard to google because it’s all punctuation!

Ron
27 Oct 2006

Ron, that’s called the ‘ternary’ operator. Basically, if you have an if/else statement like this:

if condition
  x = positive_result
else
  x = negative_result
end

You can use the ternary operator to condense that into a single line:

x = condition ? positive_result : negative_result

Many languages use this same syntax. I’m not sure where it originated, but I know it is in C.

Jamis
27 Oct 2006

I think this is the clearest description of the model, controller, view concept I have come across. Thanks.

Andy
17 Nov 2006

The Buckblog

assorted ramblings by Jamis Buck

Skinny Controller, Fat Model

Reader Comments