Some background

@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 :authenticate, :cache, and :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.

The reasons

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

Using 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 except: over 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 only: and except: is the same as adding except: []

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 30?

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.

In summary

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