One of the first things that anyone has to do in an application is assign instance variables in controllers for use in views, etc. This pattern, while dead simple, has a number of possible implementations that each have their aesthetic benefits and drawbacks. At RailsConf I had the opportunity to hash this out with none other than DHH and David Chelimsky, so I thought it might be a good opportunity to represent the different sides in a post.
The Repeat Yourself Pattern
This is where things start out many times, as they likely should. You're building out your controller for the first time, and you set the instance variable you need in each action:
class WidgetsController < ApplicationController def show @widget = Widget.find(params[:id]) end def edit @widget = Widget.find(params[:id]) end end
This method, while very clear, has the obvious problem of being repetetive. What happens when your code isn't as simple as Model.find? You're going to run into trouble quickly. So let's try a few abstractions.
The Getter Pattern (The David Chelimsky Way)
So the simplest abstraction is to do what we normally do when we're repeating ourselves: define a method!
class WidgetsController < ApplicationController def show @widget = get_widget end def edit @widget = get_widget end private def get_widget @widget = Widget.find(params[:id]) end end
This pattern has some obvious advantages over repetition: it's pretty clear what's happening while giving us the opportunity to define our finder code in a single location. However, we're still duplicating a line at the top of each action which has a very slight smell.
Note: I'm attributing this to David Chelimsky because I heard him talking about it when I entered the conversation not necessarily because he prefers it to other methods.
The Helper Pattern (The Michael Bleigh Way)
So how can we better abstract this? Well, we're going to be accessing this instance variable primarily in the view, so why are we bothering to explicitly set it in the controller? This is the pattern that I have been using for the last couple of years:
class WidgetsController < ApplicationController def show; end def edit; end private def widget @widget ||= Widget.find(params[:id]) end helper_method :widget end
This gives us a helper that we can call from our controller or our view that will lazily fetch the widget when we need it and not before. Personally I like this pattern quite a bit: I think it's relatively clear and works well in the various ways I've used it. However:
- It's a little weird that you don't see it anywhere above your actions. Might lead to confusion, especially in larger controllers.
- Via DHH: "You're calling something in the view that depends on params, which is bad."
The Filter Pattern (The DHH Way)
Finally we get to a pattern that I was aware of, but have avoided because I found it aesthetically displeasing:
class WidgetsController < ApplicationController before_filter :set_widget, :only => [:show, :edit] def show; end def edit; end private def set_widget @widget = Widget.find(params[:id]) end end
So why didn't I like this? Because before_filter seems like it shouldn't be setting a variable, but modifying things (DHH agreed that before_filter should likely have a different name such as before_action). Also I didn't like the fact that the code is split up into two places.
However, DHH made a single argument that trumped everything else I had considered: "When it comes to setters, you don't care about the implementation, but you care very much that you know it's there." And it's true: the filter isn't doing anything complicated, it's just setting an instance variable and its function is very clear from its name. So when you think of it that way, using a before_filter actually makes all kinds of sense.
Why so many?
There are other ways of achieving this pattern, even gems such as decent_exposure that can automate much of it. So why isn't there a standard solution for this extremely common abstraction "baked in" to Rails? Because at a certain point abstracting behavior that is too simple is more complex than simply not abstracting it at all.
Why did so many patterns evolve for doing the same thing? I lay the blame mostly on both the name and syntax of the before_filter method. When something is called a "filter" you expect it to be manipulating, not setting. In addition (to me at least), the :only and :except syntax doesn't feel right to use in a common case. To me, these keywords ugly up the declaration and should only be used as a last resort.
So what might we do to the Rails syntax to make this pattern a little easier to understand? Here are a few different options I've been thinking about:
class WidgetsController < Application # Option A run :set_widget, before: [:show, :edit] # Option B before :show, :edit, set: :widget before :index, run: :prepare # Option C before_action :set_widget, only: [:show, :edit] private def set_widget # ... end def prepare # ... end end
Option A adds a new run method to the controller syntax that acts as an "every filter". You could specify :before, :after, or :around options and either supply a list of actions or :all for all actions. Two downsides to this: The syntax is a little longer for the case where you want the method to run before all actions, and I don't have a good analog to :except for this option.
Option B takes a different approach and assumes that the arguments are declaring actions instead of methods to run. Here you declare one or more actions (or none for all actions) and you have two options: you can declare a :set key that will call a method called set_value where value is one or more symbols passed into the option, or you can call the :run option which will simply execute methods named as specified. This may seem like an arbitrary design choice, but Rails is full of choices that don't have specific need but rather guide developers down doing the best practice instead of doing something else. Downsides to this are again a lack of a good :except syntax and it reverses the current Rails assumption of declaring methods with before so would likely have some confusion associated if it were adopted.
Option C Is simply renaming before_filter to before_action. Obviously you would have to alias it (perhaps through the Rails 3.x versions), but eventually before_filter would be deprecated. The word filter simply implies a certain type of functionality rather than accurately describing it as simply a way to execute code before an action.
I'm not saying that any one of these options is better than what's already available, I just think that it's interesting that one of the most common patterns in Rails can still have so much difference of implementation around it. The fact that no one "best practice" has become dominant to me implies that there's some room to explore API changes to point people down a best path. What are your thoughts on the controller setter pattern?
