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?