Tracks: Part 2
Here we are, #2 in the series of articles exploring the GTD application Tracks. In part 1, Koz discussed the finer points of code reuse. Here, I’ll talk about action filters in your controllers.
I’m focusing primarily on the ContextController and ProjectController in this article—both have many similarities. Their actual purpose is not really relevant here, so I won’t describe what they actually do. Rather, let me just point out one of the first things I noticed when I started reading through the code. This pattern was startlingly prevalent:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 |
def list init ... end def show init init_todos ... end def create ... end def add_item self.init ... end |
Note that the same method gets called at the start of many of these actions. The intent here, is to do some kind of common setup, or initialization, prior to the meat of each action. Rails provides a cleaner way to do this, via filters. Filters are simply methods that get called before or after an action, and this code is a textbook example of when you ought to be using them.
In general, filters that you want executed before an action are defined via the before_filter macro:
before_filter :init, :init_todos |
Defining the filter like that would cause both init and init_todos to be called prior to every action in the current controller (and its subclasses, since filters in a superclass are inherited by subclasses).
What we want in this instance is something a bit more sophisticated. We don’t want the filters to trigger on every action, only on some of them. In fact, there are only one or two actions that need the init_todos filter. Fortunately, before_filter (and the filter macros in general) provide a very simple, but very powerful, mechanism for including or excluding actions:
1 2 |
before_filter :init, :except => :create before_filter :init_todos, :only => :show |
The above specifies that the init method should be called prior to every action, except create, and init_todos should be called prior only to the show action. Both except and only allow arrays of action names to be given, as well.
Using filters like this can help you to reduce the “noise” in your actions, leaving you free to implement only the tasty bits within the action itself. It really makes your code easier to read, and to maintain.
Another (related) thing I noticed, this time in the ApplicationController, was the following:
1 2 3 4 5 6 7 8 |
before_filter :set_session_expiration # ... def set_session_expiration return if @controller_name == 'feed' || ... # ... end |
The top-level ApplicationController specifies a before filter, set_session_expiration, that will trigger on every single action in all subclasses. It has some logic to determine what the session’s expiration should be. Note, though, that if the controller is determined to be the FeedController (the name of the controller is “feed”), that it just returns.
In general, if you need to refer to the current controller or action name explicitly in your code, you could probably refactor it to be cleaner. In this case, you can let Rails’ filtration code do the conditional work for you. In ApplicationController, simply omit that condition altogether. Then, in the FeedController, do:
1 2 3 4 5 |
class FeedController < ApplicationController skip_before_filter :set_session_expiration # ... end |
session :off. This will disable sessions for that controller, and stop your sessions table from filling up with empty, aggregator-generated sessions.
The skip_before_filter macro allows subclasses to override filter behavior configured in their parent class. Here, we tell Rails that we want to “skip”, or ignore, the set_session_expiration filter for all actions in this controller. (You can use the only and except parameters here, as well, if you need finer tuning.) By using the skip_before_filter macro, the code that applies to the FeedController is in the FeedController, and the ApplicationController remains blissfully unaware of its subclasses.
(While we’re on the topic of filters, I might just add that filters in edge rails have one minor drawback: they cause your stacktrace to become ridiculously long, because they are processed recursively. A way to process them without making recursive calls would be a wonderful patch. Hint, hint!)
Nice, I like how this one weaves the old code, suggestions, and new code together in an accessible manner – definitely starting to hit on all cylinders.
Thanks, Tim. We’re learning!
These posts age great. I’m looking forward to more!
Great work! I found this post a easier to follow than the last one, but it is also discussing a simpler concept which may be part of the reason. I like the variety. Keep it up.
Thanks a lot for starting therailsway.com
These articles not only show you rails ways (if you not following pattern). its also help to gain confidence that the way you code is rails way (if you coding correct)
And also like to see good ruby tips as most developers are learning rails and ruby together.
Niket,
We’ll definitely make an effort to cover ruby-style too, but we can only cover what people submit. If you have some ruby code you feel isn’t all it could be, let us know.
So is the ridiculously long stack trace in Edge Rails a side-effect of an otherwise improved filter processing implementation?
Clear, concise, easy-to-follow. Excellent!
John, the long stack trace in edge rails is partially the fault of the new filter implementation, yes. The more filters you have for a particular action, the longer the trace is.
Why I agree that before_filters can be very useful, I don’t necessarily think they should be used to the extent in this article. For example, if they’re used throughout an application, they can surely save a lot of extra work and also avoid some pitfalls (before_filter :require_login).
However, if they’re used within a single controller, it’s sometimes better to just call them each time for code readability. For example, in this case the “init_todos” only gets called once. Why install a before_filter? That could only serve to confuse the hell out of someone reading the code in “show” They might ask—well, how is this getting setup? They’d have to pan to the top of the controller code to see that some before_filter was installed.
Basically, you’re taking some code out of context whenever you do this. It needs to be done in an intelligent manner and not just to show off the fact that you have this nice thing called a before_filter. They keep things clean, but only really to the people who can wrap the head around the full project. In a larger sense where you have several people reading/writing code, it can confuse things.
The problem with patching the current code to avoid an excessively large stack is that you still have problems with around filter. Around filter by nature has you call ‘yield’ in the middle of the filter, which will increase the stack size. Either the stack takes a hit or we have to come up with an alternate hook for people to use in their around_filter actions.
Shalev, sure. But (in my case, at least) around_hooks are the exception. In fact, I use before_filter almost exclusively. It’d be nice if there was a way for before/after filters to not need the recursive call.
Once it starts hurting me enough, I’ll look into it. In the meantime, I keep hoping someone else reaches the intense pain threshold before me.
Koz wrote:
The FeedController currently does:
session :disabled => true, :except => 'index' # Prevents session control from interfering with feedIs that along the lines of what you’d recommend, Koz?
Yep,
That’ll work a treat luke. You could also move the action which displays the list of feeds to another controller, and keep the feed controller just for feeds. Then your before filters, and session declarations get much simpler.
I wonder if the before_filter is the answer to a question Ive been asking myself for awhile now. I have a number of models with belongs_to associations back to a User model. I really appreciate the elegance of Rails way of grabbing an array of Foo objects that are associated with the user, User.foos. My concern is that in my user’s id is stored in session, so in each method I seem to be querying the db for the user object and again querying the db for all of the foos, or bars, or whatnots. Could I use the before_filter and possibly cache the value of the User.find(session[:user]) query to pair down the number of db requests?
Aaron, that’s a very appropriate use of before_filter. Every single application I’ve written uses that pattern. You have a before filter that runs for every action, which grabs the id out of the session and looks up the user, storing the result in an instance variable, @user.
Thanks for writing this up Jamis. So I have a question about the functions that rely on the before_filter.
Lets say I have three functions that all they do is get a @job ready for the view to have its way with. So I put @job = Job.find(params[:id]) into the before_filter
and now i have three empty function defs that are essentially empty because the filter did all the work. is that bad ruby style?
before_filter :get_job
def files end
def comments end
def links end
is there any better way to do this?
binh, Rails is actually pretty intelligent sometimes—if you have an action that doesn’t actually do anything, you can omit the definition altogether. When Rails gets a request for an action that doesn’t exist, it checks to see if there is a template defined with that name, and if so, it just renders the template directly. Thus, you can drop the “def files” and friends completely.