Are filters abused in Rails?

alternate_method_controller.rb

class MyController < ApplicationController
  def show
    @model = load_model
  end

  def edit
    @model = load_model
  end

  private

  def load_model
    Model.find(params[:id])
  end
end

filter_controller.rb

class MyController < ApplicationController
  before_filter :load_model, :only => [:show, :edit, :update, :destroy]

  def show
  end

  def edit
  end

  private

  def load_model
    @model = Model.find(params[:id])
  end
end

helper_method_controller.rb

class MyController < ApplicationController
  helper_method :model

  def show
    # Use +model+ freely
  end

  def edit
    # Use +model+ freely
  end

  private

  def model
    @model ||= Model.find(params[:id])
  end
end

method_controller.rb

class MyController < ApplicationController
  def show
    load_model
  end

  def edit
    load_model
  end

  private

  def load_model
    @model = Model.find(params[:id])
  end
end

Everybody vote! http://gopollgo.com/are-before-filter-for-loading-models-in-rails-controllers-a-code-smell

This was inspired by my colleague @lucabelmondo's comments during recent code reviews. Without putting words in his mouth, he made me consider whether filters are abused in Rails.

Should filters only be used for things like authentication which prevent an action from firing altogether or set up context (such as the current user) that could be argued to be outside the responsibility of the action itself?

If this is true then the common pattern of doing set up of instance variables might conflict with this idea. Sure, it saves duplication but does it make it harder to find what variables are set and where? Does it become even worse with inherited filters (e.g. a variable set from ApplicationController)?

The final question really is the ever inflammatory "is this too much magic?" The ability to somehow augment a method without using inheritance isn't something normally available and things like alias_method_chain have come under fire in the past for this same reason. Would something like Jon Leighton's suggestion to have objects for actions make this better at all?

I'm not a Rails expert but from a code readability and maintainability standpoint, it seems slightly inappropriate to use filters to load the model so not too keen on the approach in https://gist.github.com/2007669#file_filter_controller.rb

Model loading is an explicit part of the logic of the CRUD methods and that's where it should be placed (especially if its just a simple find) so that the logic is clear and can be followed easily instead of consulting a filter which might have been applied or not and which might not even be in the current controller. Filters seem most appropriate for cross-cutting concerns such as security and logging and other boiler-plate code which is not directly related to the logic of the operation. Using the DRY principle to justify filters seems inappropriate as they can lead to code which is much harder to understand e.g.

def show
end

I would prefer to use the approach in https://gist.github.com/2007669#file_method_controller.rb as this offers an appropriate level of abstraction

The "Don't Make Me Think" pattern could be applied to source code.

One of Krug's fundamental ideas is that in the context of a website, regardless of the page you land on, you should be able to immediately find your way around.

Applying the same principle to code, a method or an object should ideally be able to tell you a story without excessive digging. It's not easy to do this when using filters.
Of course is a good idea to make common setup (like setting instance variables) when a instance is created. And it's OK doing this in a superclass.
But unlike the initialize hook, filters can be applied selectively to some actions and not to others, even skipped in subclasses, with endless combinations.
This seems to suggest that we are not using the filter to initialize something, but moving the implementation of some methods to somewhere else.

@jonleighton's idea of single-action controllers cited by @mudge is interesting indeed: it made me realize that we always use only one action of every controller instance for every request. Because when we say render :other_action we are not actually calling that method, but just rendering the corresponding template. When redirecting, we are creating a new request/response cycle, and therefore a new instance of the controller.
Basically every action behaves like an object.

From this perspective filters could actually be considered an antipattern, and what they do could well fit the initialize in traditional OOP.

Of the three options, the first one, https://gist.github.com/2007669#file_alternate_method_controller.rb, is probably superior, because explicitly declares the variable. It works well especially when there is a single object defined. In more complex scenarios we could consolidate all the data necessary to render a page in a single presenter object, instead of creating different instance variables, as we would do when serving a complex JSON response.

Regarding authentication and authorization and how they are often implemented with filters, it's interesting to look at what Padrino does.

In a typical Padrino project, the root directory contains at the same level models, admin and app, created by different generators.

.
├── admin
│   ├── app.rb
│   ├── controllers
│   │   ├── ...
│   │   ├── products.rb <== CRUD
│   │   └── ...
│   └── views
│       └── ...
...
├── app
│   ├── app.rb
│   ├── controllers     <== not predefined
│   │   └── product.rb
│   ├── helpers
│   │   └── product_helper.rb
│   └── views
│       ├── layouts
│       └── product
...
├── models
│   ├── account.rb
│   └── product.rb
...

models are therefore trasversal to the project, the app is just the public part of the project with no actions predefined by the generator, and finally the admin contains the CRUD operations where everything is authenticated by default.

This is just to say that it's considered good practice having a clear separation between admin and public side of a web application.

In fact this pattern is also common in Rails, where we namespace the admin controllers, and derive them all from a single superclass where the authentication is ensured with a filter.
This architecture probably allows to replace the before_filter :require_user with initialize which could contain a call to super as well as to require_user, returning to a simpler and more rigorous OO pattern.
Not sure about the latter, though, because strange things could happen when we touch initialize in Rails, like in the ActiveRecord classes... :-)

Thanks to @mudge for this interesting discussion.

It's also worth noting that the original definition of the DRY Principle in "The Pragmatic Programmer" is as follows:

Every piece of knowledge must have a single, unambiguous, authoritative representation within a system.

None of the examples above violate this as, even though there are repeated method calls, the actual knowledge ("load a specific model identified by params[:id]") is not duplicated.

I've been trying to remember a time when I've been bitten by not knowing how an instance variable has been defined in a controller and I can't think of one. So by default I think I'd probably opt for the before_filter option. It's enough of a rails idiom that it doesn't take too much knowledge to figure out what's going on. This is especially the case if you have a FoosController which does Foo.find: anyone with any Rails experience will know this pattern. It's even the basis of gems like inherited_resources.

I'm coming round to the idea that application_controller shouldn't define instance variables. Instead you could just use regular methods and make them helper_methods if you need them in views. That would at least remove some of the dangers of accidentally overwriting instance variables through inheritance.

The single action controller idea does sound interesting, but I'm not sure I agree with @lucabelmondo that the filter behaviour can be easily captured by the initialize method. With the initialize method you lose the ability to skip filters, which means each action would probably have to define each filter on every action explicitly. At some point, you would have loads of duplication in your actions and you'd probably start to abstract some of the more common ones. And then you're reinventing filters! So maybe the skip_before_filter pattern is the flaw. But I can't think of a way of removing skip_before_filter without introducing lots of duplication elsewhere.

We've been bitten by overzealous filters in ApplicationController recently (e.g. redirecting users to a mandatory landing page if they haven't seen it before interfering with API calls, etc.) and maybe that's the real problem here: a sort of action at a distance. Perhaps having a policy of not setting filters in superclasses is enough to keep the filter chain obvious?

The problem with this is that it is nice to use the following pattern:

class Admin::AdminController < ApplicationController
  before_filter :admin_required
end

class Admin::FoosController < Admin::AdminController
  # Every action is automatically filtered
end

I guess for me it's not really action at a distance. It's really just a constraint on your app, in the same way that auth or logging or other cross-cutting concerns are. So to me that does feel like it's a good candidate for a global filter. When you write controller tests for a new action it'd fail at first, and that's when you'd have to decide whether to skip it or not. That's good, because it forces you to consider the landing page every time you write a new action. I suppose you could also mitigate it by only redirecting to the landing page on GET requests and for :html rather than :js/:xml formats.

It's also interesting to see how Ramaze approaches the problem, namely relying on calls to super http://ramaze.net/documentation/file.controllers.html
I make no mystery of my predilection for Ramaze over Sinatra.

@ohthatjames I agree that a test should fail in this case but I'm not sure about using the presence of a safety net as your sole way of informing developers of the design, viz. someone could ostensibly create a new action and you'd be entirely reliant on your test coverage to teach them how the system works. Can we not hope for greater clarity in the code itself?

I'm reminded of another comment of yours regarding avoiding deeply nested contexts in tests for this same reason: the lack of transparency about what set up is actually occurring.

Following the Ruby Rogues discussion about Objects in Rails, I've added another approach: using helper_method to avoid exposing instance variables altogether. This has the added benefit of working across views.

In the common quest of a more better approach, the helper methods are a quite a interesting approach.

The helper methods though, as intended by Rails, break the encapsulation.

They also represent a problem for the single responsibility, because their presence is not always required by the controller instance, but dictated by the needs of the template.

As "cross object" instance variables which belong to 2 different objects at the same time, they are somehow pseudo-global entities instead of being injected.

When it comes to testing, they still need to be stubbed out as part of the context.

Also, if they assign instance variables, the latter will be again available in the view, representing a further temptation that only the developer's discipline can prevent :-)

Unfortunately all this is due to the very nature of how Rails works.

Furthermore, it's also related to the legacy of the original ERB implementation from the standard library. An ERB object only accepts a Binding object, and raises an error if the argument is of a different type. That probably encouraged this historical instance variable based approach in Rails.

Erubis (the default ERB engine as of Rails 3.0) does better in this respect, because you can also pass a Hash to the result.

template = Erubis::Eruby.new("<%= omg %>, this is a <%= message %>!!!")
object_to_be_rendered = {omg: "ZOMG!", message: "different kind of template"}
template.result(object_to_be_rendered)
# => "ZOMG!, this is a different kind of template!!!"

This allows to decide to rely on only one simple context object for the view, and this encourages to build that object as dumb as possible, probably a presenter on top of a database-backed model in the Rails common scenario.

This approach seems closer to the one we find in the Javascript templating systems. For me it has the advantage that I can easily stub it out not as pseudo-global context (difficult) but as a parameter to pass to the subject under test (easy).

It's also interesting that in we can get something similar in Rails making explicit use of the :locals option

render :template => "my/template", :locals => {omg: "ZOMG!", message: "different kind of template"}

The issue is how would you deal with global data. Like menu/cart etc if not using before filter