@d = 25
Here at The Frontier Group we have recently started using Rails 4 for our new projects and even migrating a couple of older ones. It’s taken a little while, but we feel that it’s been out in the wild long enough, giving a chance for most of the major bugs to be weeded out. Amongst many of the new features, one is
before_action which is a new name for the trusty old
before_filter. It’s my opinion that renaming it was a bad move as it encourages misuse. This is a controversial opinion to hold; even within TFG. But in any case, before jumping to the comments to tell me of my errors, let me make my case.
In the beginning, I believe the intentions of the humble
before_filter were pure. They were to provide a method to prevent an action from ever running; effectively filtering the action before it runs, hence the name. This seems to be supported by the ActionPack README from 1.2 up until 3.2. As of 4.0 that README becomes quite sparse. If you don’t feel like looking at seven year old documentation, examples of
before_filter are used to invoke methods such as
:audit. Suspiciously missing are examples using
before_filter to load instance variables such as
before_filter :find_post. In fact, the examples of how
ivars are used to link the controller and template look like this:
def show @customer = find_customer end def update @customer = find_customer # more stuff down here end
I suspect that the abuse of
before_filter started, or at least became popular when gems like CanCan started to emerge. For those unfamiliar, CanCan provides a method called
load_and_authorize_resouce which does pretty much exactly what it says it does. It loads a resource then authorizes an action upon it. Should the current user be un-authorized to perform the action, it doesn’t get executed. Presumably much the same way as
before_filter :authenticate from the ActionPack Readme would do. With one caveat, it also loads a resource into an
ivar of the same name. This leads to our new controller looking like this:
load_and_authorize_resource :post def show # @post has been set CanCan end def update # @post has been set CanCan # more stuff down here end
Coupled with the ease provided by CanCan, and one of the most overused acronyms and default go to response that I’ve seen since working with Rails (that’s DRY btw), this idea exploded. Note that I actually have NO data to back this up; it’s just speculation. Regardless of the actual cause, I now see code like the following:
before_filter :find_posts, except: [:show] before_filter :find_post, only: [:show] before_filter :find_commenters, only: [:show] def index() end def my_posts() @posts = @posts.created_by(current_user) end def show() end private def find_post() @posts = Post.find(params[:id]) end def find_posts() @posts = Post.all end def find_commenters() @post.comments.map(&:creator).uniq end
This is absolutely terrible code, all for the sake of DRY. At least in Rails 1 through 3, there was an indication that you were doing it wrong via the method name due to the lack of any form of filtering. Now with
before_action it seems to be encouraged.
First off, the reasons arguing that it’s good.
For: It’s DRY
This code is very DRY. There is no repetition to be found here. In fact there is nothing in the methods at all, so there is nothing to repeat. Of course the benefits of not duplicating code are well documented and proven. If there is a bug in that code, there is only one place to fix it; saving time, effort, and you’re note going to forget to fix it in *that* other place.
For: It’s the Rails way
before_filters in this way seems to have become the rails way, with rename to
before_action adding legitimacy. There is something to be said to doing what other people expect. It means that they can come into your work and know exactly whats going on. And indeed that’s true, deviating from convention can lead to some level of confusion. So if you intend on doing something other than the convention, you should have a sound reason.
On that note, why do I dislike the current usage?
Against: It’s not anymore DRY
Just above I mention that using DRY as a reason to use
before_actions. And in fact DRY is a go to reason for many things,
before_actions not withstanding. The only issue is that you are still repeating yourself with
before_actions. Note those pesky
only: keys, they violate DRY as exactly the same amount as calling the method in your action. You just swap what you’re repeating. In one case you repeat the action name, in the other it’s the name of the
filter (using the term lightly) method.
I would propose not setting the variable inside that method, and in which case you do end up repeating the variable name. But you don’t have to, in order to stop using
before_action. Compare the two code samples below:
before_action :find_post, only: :show def show end
is equivalent to:
def show find_post end
In fact, it’s even shorter doing it in the action!
Against: Except is terrible
Admittedly you can get away from repeating the action names by using
only:. Black lists always leave a nasty taste in my mouth when it comes to coding. They presume you know any future uses, or can ensure that any future maintainer is aware of the list and it also needs updating. You can’t be sure that your before action isn’t going to blow over the results of another before action (see my point on side effects below), or admittedly a less sinister action of loading records that aren’t required.
Note that omitting both
except: is the same as adding
Against: They abstract the flow of the action from the developer
It’s quite obvious they occur before the action does; after all it says it in the method name
before_action. What isn’t obvious is how they play with each other. Looking at the example above, note where the order of execution is defined and where it actually matters. Also all actions need to have consistent input parameters, see how
show doesn’t have any control over which post to actually show.
Against: They elevate the live time of the variables
This ties pretty closely to my previous reason. Code Complete discusses the concept of live time (hopefully that link works for you all). The basic concept is that, the further variables are defined from their usage, the more difficult the code becomes to maintain.
@a = 2 @b = 3 @c = @a + @b
The value of
@c should be obvious to all. However what about
@c + @d? How easy was it for you to say
Against: They have to cause side effects
I dislike side effect causing functions. There I said it. I’m a side-effectist. Every chance to I get to eliminate one is a little personal victory. Eric Evans has a pretty nice explanation of their pitfalls in his book Domain Driven Design. I’m not going to preach benefits the benefits of side effect free functions, except to say that any method that changes state introduces a chance that, that method will be used without knowledge of that side effect. On their own, there is nothing wrong with side effect causing methods, we can’t do our job without them. However they do compound complexity, as you need to understand the side effects of every method in the call chain. You should consider if you really do need to modify state in a method before doing so. To make matters worse, these methods often have un-assuming names, like
find_post, which provides no indication state will be changed.
Against: Actions must rely on the side effects of other methods
By using a
before_action to configure state, you remove that responsibility from the action. However the sole purpose for an action to exist is to configure, and maybe work with, that state. In effect you are robbing the action of its only job. The first place you look for action code is the action itself. It is not acceptable that a developer should be expected to have to search the entire controller, and any it inherits from, to discover how/why an action is/isn’t working.
I would love to see the use of before filters/actions returned to their (in my opinion originally intended) use of preventing actions from executing. And use of them solely to load data banished to the annals of history. Code such as the following, despite being slightly longer, reads far better, and is easier to comprehend and maintain:
def show @post = find_post authorize!(:read, @post) @commenters = find_commenters_on_post(@post) end def update @post = find_post authorize!(:update, @post) # update it! end