Skinny Controllers, Skinny Models

Posted by Joe Ferris

May 01

I hear a lot of people recommending the “skinny controller, fat model” approach to Rails development. I’m all for keeping controllers simple, but who wants a fat model? If your editor slows down while loading up your model files, I have some advice: put your models on a diet.

Let’s say you have an application that needs to handle PDF documents. You have a very simple Document model to keep track of them:

class Document < ActiveRecord::Base
  validates_presence_of :title
  has_attached_file :pdf
  validates_attachment_presence :pdf
end

It’s just you and a skinny, attractive model. It’s going to be a good day.

But after your application has been live for a few days, it becomes clear that you need to provide a way to view these documents online, and your client’s weapon of choice is HTML. So, you add a method to convert your PDFs to HTML documents:

class Document < ActiveRecord::Base

  # ...

  def convert_to_html
    # ...some fancy magic...
  end

  def converted_to_html?
    File.exist?(html_file_path)
  end

  def html_file_path
    File.join(HTML_STORAGE_DIR, pdf.original_filename + '.html')
  end

  # probably a few more methods...

end

Everything is working great, but now you have to look through all this HTML junk whenever you’re working on Document. Worse, the tests for HTML conversion and documents are all mixed up. Even worse, Document is getting pretty fat, and its model friends won’t stop making fun of it. If you don’t do something soon, this could mark the end of your good days with skinny models. A very common and simple technique can save us from this message: composition.

For some reason, many Rails developers seem to avoid using model classes that are not stored in the database. This leads to shoving too much key functionality into one of your key models, which of course leads to fat, incomprehensible model files and tests. I see no reason for an HTML file to have its own, separate entry in the database, but it certainly has enough behavior to warrant its own class. Let’s pull that functionality into an HtmlFile class:

class HtmlFile

  attr_reader :source_path

  def initialize (source_path)
    @source_path = source_path
  end

  def name
    @name ||= File.basename(source_path) + '.html'
  end

  def generate
    # ...some magic with file.path here...
    self
  end

  def path
    @path ||= File.join(HTML_STORAGE_DIR, name)
  end

  def exists?
    File.exist?(path)
  end

  # ... some other useful methods ...

end

Now we have another beautiful model. Let’s get these two acquainted:

class Document

  # ...

  def html_file
    @html_file ||= HtmlFile.new(pdf.original_filename)
  end

  def convert_to_html
    @html_file = HtmlFile.new(pdf.original_filename).generate
  end

end

Result: two beautiful, skinny models. Analysis: it’s going to be a very good day.

Thoughtbot

thoughtbot is a technology consulting firm that provides web application development and design services. We focus on building modern systems, embracing good ideas and delivering elegant solutions.

Interested in learning Rails?

Comments on this post

leethal said,

I bet feminists will hate this post.

Which is a shame, cause it’s really awesome!

Brian said,

Why not include or extend the html file class rather than instantiating it. You’re not really creating the html file, you’re just formatting the model. It really is a good use case for a presenter. Also this is a reason I love datamapper. With inline property declaration you can have modules that will define structure as well as behavior.

Jeremy McAnally said,

I agree with Brian here. Using modules, or, if you’re fancy, something like “concerns,” to design your classes solves the problem of cluttering your class code with too much functionality while also keeping your object model tight.

I’m pretty anti-”extra classes because one is too big even though the functionality really belongs there.”

Trevor Squires said,

Jeremy,

“functionality really belongs there” ... I have to say that any time I’ve ever heard that argument it’s been keystroke-driven rather than truly functionality-driven.

123	box = CardboardBox.new(:contents => :stuff)box.deliver_to(:germany)

Yes, being able to call the deliver_to() method on a box looks pretty expressive but in my opinion, the job of a cardboard box is to hold stuff.

“Telling a box to deliver itself to Germany” is my favorite example of why everything should be skinny.

Pretty much anyone can see that a real-world system of actors doing stuff to the box is more efficient and easer to understand – even if it does involve a lot of different actors.

That said, code written with a pathological commitment to eliminating SRP violations would probably be just as ugly to work with as code written without giving it a thought.

But if you’re going to lean towards anything, SRP isn’t the worst thing you can do.

Oh and by the way. If a module has only one consumer then you’re not cross-cutting anything – you’re simply hiding bloat and SRP violations.

So if the first thing you reach for when trimming the fat from a given class is “how about putting those methods in a module” then you’re wrong. There. I said it.

jareCare said,

Trevor,

You tell the box to deliver itself, thats its responsibility. Oh what we need a BoxDeliveryService?

Don’t turn methods/behavior into classes. If you do you get your “actors” or what I like to call “services”.

MessagingService#send(message)

Message#send

PersistanceService#save(domain_object)

DomainObject#save

Keep creating “actors” and “services” and you’re going to end up with a bunch of objects that are nothing but getters/setters with all their interesting behavior in other classes. In your example, that CarboardBox is nothing but a glorified collection; worthless.

Jon Yurek said,

The thing is that in Ruby we have wonderful the wonderful Module construct that can be inserted anywhere and brings functionality along with it. If functionality truly doesn’t belong somewhere, but doesn’t make sense that it should be instantiated, then a module is a perfect place, especially when it can be use in multiple classes. You don’t need a DeliveryService instance to deliver the CardboardBox, it just needs to include Deliverable.

I think the point about the presenter is a good one, and one I admit I didn’t think of before this went out… displaying the PDF as HTML doesn’t seem like a “model’s” responsibility and so is iffy as a class. But it does make sense to put that functionality somewhere other than the business logic of the model so it doesn’t obscure important functionality with presentational logic.

Really, the two (a class and a module) are functionally equivalent in the end, it’s how you want to think about the problem that effects the final code.

Justin said,

Finally some substance on this blog I can jive with…

Modules, extra Classes, Observer classes, Namespacing, even DSL development… these are all things that should be weilded at the discretion of the programmer. He is always working in an environment in which he needs to control the balance and extendability of his code.

There is no right or wrong way.

Only the programmer knows when, where and how he should apply his code. He should be aware of his surroundings enough to see that others on the team may even want/need this type of functionality as an extra class/module. What ever packaged form it should take. Too many bad programmers don’t see ahead, or know when the right time is to do this sort of thing.

So yes, this is one of those 8 bazillion possiblities. I also believe you were dead on, as far as most people not keeping in mind to abstract out and refine their overloaded classes.

May 02

jareCare,

“You tell the box to deliver itself, thats its responsibility. Oh what we need a BoxDeliveryService?”

Yes we do – this is why I use the example. The implication is that delivery to Germany is a complex behavior compared to just being a cardboard box. It’s why we have services like FedEx or DHL rather than boxes with wheels or wings. A box has enough to worry about with issues like “are my contents within my maximum capacity” without having to worry about “I need to book an airline ticket to Frankfurt”.

Just because behavior touches an object doesn’t mean the behavior is “its responsibility”. And just because an object’s set of behavior is comparatively limited doesn’t make it ‘worthless’. Starlings are ‘only’ good at tracking seven of their neighbors in flight, making the emergent behavior of a flock astounding.

The ‘service’ examples you bring up are very close some of the ugly code I’ve had to work with in the past. However, they are (probably) not so much examples of a pathological commitment to SRP (which I said would be just as ugly to work with as code that disregarded it) as they are examples of code that is (again, probably) hiding pluggable implementations behind a generic interface. Don’t throw out the baby with the bathwater just because “pluggable implementations” turned out to be a feature that wasn’t needed after all.

There’s absolutely nothing wrong with objects that do nothing but push other objects around.

The whole Box/Germany thing came about after inheriting Rails code with several model.rb files in excess of 500 – 1,000 lines. The worst offender really was the conceptual equivalent cardboard box that knew how to get itself to Germany. Moving lines of code into modules would do nothing to address the fact that the ‘box’ was simply doing too much.

It’s an equally ridiculous contra-example to your “everything as a service” examples but it’s important to understand how they got there: one behavior at a time.

Which is why your position sets off alarm bells for me. “Don’t turn methods/behavior into classes” is a principle for recognizing when you’re taking SRP too far, not a justification for piling “just one more behavior” into a class.

Jon’s point about “obscuring important functionality with presentational logic” is exactly what I’m talking about. I’m probably more dogmatic in that I’d say “cluttering relevant behavior with orthogonal behavior”.

The divide isn’t just about business vs presentation – it’s about defining and limiting the intrinsic purpose of your classes. It’s hard, contentious and, if you nail it, a little bit like watching the interactions within a flock of birds.

Thanks for the discussion guys.

Tammer Saleh said,

@trevor – I think you make some great points. The most important being that this is a finicky matter of style/opinion. merits.

I personally agree that in your example, a Box shouldn’t know anything about the complexities of Delivery, but I also don’t have a problem with moving that behavior into a module instead of a new object in the domain. The important outcome of isolating the behavior is achieved in either case.

Tammer,

it certainly isolates the lines of code, but it doesn’t guarantee that you isolate the behavior.

Sensible definition of boundaries aside, if ‘box’ responds to methods related to Delivery (by virtue of an included module) then you will have to test/spec that aspect of Box behavior.

Which would lead me to ask – if you don’t think Box should know about the intricacies of Delivery, why are you having to test Boxes behaving like something that knows about the intricacies of Delivery?

When I include a module in a class it’s because I want to add the behavior to the class, with all the attendant responsibilities it brings (like making sure I’m not corrupting the intrinsic purpose of the class) .

I hope this all doesn’t sound like I’m ragging on you guys. However strong my opinions are, I know that they are just opinions.

May 06

Sergio Pereira said,

Hey, that’s like trading your supermodel for a group of skinny models. I like it. I always found supermodels to be too high maintenance.

Rabbit said,

Awesome. I love it. Trevor and Joe are heroes in the way Alan Kay and David West are heroes.

Objects with minimal responsibilities, and only as defined by the problem domain. Period.

Slim models with clearly defined, easily understood behaviors that interact with a multitude of other models to collectively perform an otherwise complicated task.

@Jon Yurek re: modules being wonderful.

Yes, they’re wonderful, but I think creating a module to transfer methods from an object that’s getting fat is premature optimization, whereby optimization here is defined as “re-usability”. Are those methods in that module really able to re-used by other objects?

@Trevor re: starlings in flock.

Emergent design. :)

Also, what’s an SRP violation?

May 08

Koz said,

This is an excellent post, with which I wholeheartedly agree.

So long as what you’re talking about is simply increasing the granularity of your object model. Zooming in on ‘user’ to expand it into ‘user’, ‘customer’, ‘author’ is a great idea if you find yourself with user which does everything in your application.

But don’t be so quick to assume that a busy class is one that’s too busy. In any particular domain something’s going to be the busiest, just like there’ll be people with one particular job description doing the largest number of distinct tasks.

May 22

Matt Deiters said,

I couldn’t disagree with Koz more, the most fundamental aspect of real object orientation is objects with a single responsibility. (See: http://en.wikipedia.org/wiki/Single_responsibility_principle). Objects are not people, that line of reasoning would model an accounting system into two objects – the accountant and the banker.

Modules are nice but the reality is they are multiple inheritance and lead to tightly coupled behavior if misused. To illustrate, treat yourself sometime to they internals of ActiveRecord. Its utter mess in there…

Sorry, comments are closed for this article.

Come “ride the toad” on Hoptoad, the app error app.
Thunder Thimble: Brand monitoring for social media.
Widgetfinger: Simple content management for simple websites.
Tee-Bot, funny shirts your friends won't understand!
Umbrella Today: “It’s like totally the simplest weather report ever, Julie.”

GIANT ROBOTS SMASHING INTO OTHER GIANT ROBOTS