Thin controllers isn’t a matter of line count

Controllers are the part of a Rails app nobody really wants to write. “Thin controller, fat model” has been the watchword of Rails devs for years (and is still worthwhile, if you don’t assume that ‘model’ means ‘ActiveRecord::Base subclasses’). Model code is fun to write: it’s where the interesting problems are, after all. View code is fun to write: it’s where you get a chance to be creative and think about appearances. But controller code is boring. It’s riddled with boilerplate and 90% of the time consists of dumping data from your view into your model and vice versa. So there’s been a lot of work done on making controllers as tiny as possible – Rails features like respond_with and gems like inherited_resources. People push up boilerplate logic into before filters and into methods on ApplicationController, and you end up with controllers with just a handful of lines. Look at this example from Inherited Resources:

class ProjectsController < InheritedResources::Base
end

Isn’t that the Rails dev’s dream? To not have to write shitty, boring controller code at all?

I’ve come to believe that these approaches to paring down the code size of controllers are counterproductive, for several reasons. The biggest of these is that you’re ultimately increasing the size of the controller’s behavior – the logic is more complex and it’s harder to look at the class and explain what it does. I think a controller action should be a readable narrative about how the user’s intention (expressed as the action) is applied to the model. The guidelines I’m trying to adhere to:

  • Avoid inherited behavior.
  • Use methods, not instance variables.
  • Use filters for filtering, not for retrieving data.
  • Share common behavior through composition, not inheritance.

Avoid inherited behavior. It’s too easy to end up with one or more mudball ancestor controllers – ApplicationController, AdminController, that kind of thing. These classes end up having a stew of methods and filters which apply to a subset of their descendants, but – critically – have nothing to do with the behavior of ApplicationController. ApplicationController should only contain logic which will be used by every controller in your system. If you find yourself using skip_before_filter or any other kind of workaround to disable global behavior, back off and consider whether that behavior is genuinely global.

Use methods, not instance variables. Developers will sometimes use filters to ensure that various objects are available to actions and views in the controller. These objects are stored in instance variables and referred to in the controller, in views, and in helpers. This kind of code will frequently have a lot of nil checks:

if @google_analytics_code
  render 'google_analytics_code', :code => @google_analytics_code
end

I’m in the camp that wishes Rails required you to explicitly pass objects to views, but short of that I prefer to define methods and make them available to the view if necessary. So instead of this:

class SomeController < ApplicationController

  before_filter :set_google_analytics_code

  # ...

  def set_google_analytics_code
    @google_analytics_code = Settings.google_analytics_code 
  end

end

I prefer this:

class SomeController

  # ...
  
  private

  def google_analytics_code
    Settings.google_analytics_code
  end
  helper_method :google_analytics_code

end

This has two major advantages. First, it allows your views to automatically differentiate between the value not being set and having not been retrieved. In the example above, a view is expected to embed the Google Analytics code if one is set, but otherwise to do nothing. But @google_analytics_code will be nil in two different cases: if Settings.google_analytics_code is not set (which is acceptable) and if the set_google_analytics_code helper never fired (which is a bug). The view has no way to know if the controller is doing its job, and you may end up with a situation where a feature is quietly failing to work. On the other hand, when the view calls google_analytics_code, it’s making a distinction between an expected absence and an accidental one – because the latter will raise a NameError.

Second, it means that in situations where the method isn’t having the behavior you expect, you can at least be certain where to look. If a view calls google_analytics_code and doesn’t get the value it expects, the problem is between the beginning and end of the google_analytics_code method. If @google_analytics_code doesn’t have the value you expect, it might be because of that set_google_analytics_code before filter. Or it could be a helper method that’s accidentally setting that variable. Or some partial somewhere. Or another before filter that accidentally got put lower in the chain, overriding set_google_analytics_code entirely. This might seem like a silly distinction in this contrived example, but in larger apps (and with more complicated data than Google Analytics codes) the difference can mean hours of debugging.