Never use ActiveRecord persistence methods in Rails controllers
And never, ever use ActiveRecord filters.
Rails controllers are a mess.
An ongoing discussion in the Rails community is how, when, and if to test controllers (and how impossible is is to write tests for controllers that are both thorough and not completely isomorphic with implementation).
There are many dimensions to this problem, and it touches a lot of the philosophy of what makes rails rails and what best practices should be: how fat or skinny the controller is, how OO the overall architecture is, where the domain logic lives.
Of all the discussion, not much in the way of practical solutions have emerged. There are some really cool experimental projects (e.g. Focused Controller, Poniard, and another notable one I'm forgetting the name of, starts with a "C") and now a brand new framework that shows much promise, Lotus. These require complete adoption of a new paradigm and don't do much to help the everyday rails project have better code.
There is, however, one set of ideas that has emerged from this discussion which is very easy to implement and immediately offers benefits with essentially no drawbacks:
- Don't use ActiveRecord persistence methods in controllers.
- Don't use ActiveRecord filters.
Instead, write ActiveRecord class methods which do the set of persistence and other related tasks in one logical place. Then, use those methods from your controllers.
class Thing def self.make_new(creator, params) thing = new(params) thing.creator_id = creator.id thing.save! Stats.count("Thing Created", 1) thing end def self.find_and_update(id, params) thing = find(id) thing.update!(params) thing end end class ThingsController def create thing = Thing.make_new(current_user, params[:thing].permit(:foo, :bar)) Thing.delay.send_notifications(thing.id) @thing = thing.decorate end def update thing = Thing.find_and_update(params[:id], params[:thing].permit(:foo, :bar)) @thing = thing.decorate end end
By itself, this approach has greatly improved my code in the following ways:
- It's easier to to reason about what
find_and_updateare for and their role in the system.
- It moves domain logic into the model.
- If I need to make or update a
Thingoutside of a controller (like in a script or at the command line) I can do that. Before, when DRYing up this controller code or splitting it into methods for better organization, I would create these methods as private classes on the controller -- inaccessible outside of the controller and bad OO design.
- No more filters to mentally chase around -- hooray! If I want to add functionality to what happens in a save or update, I put that directly into these methods.
- I can test
Thing.find_and_updatein a completely functional matter.
- Making changes to what happens during a creation or update is much easier -- I can do it completely inside of the model without thinking about controller behavior changes (and if it does cause controller behavior change, then the change should probably go into the controller).
- I can stub out the behavior of
Thing.find_and_updatein my controller tests, making these tests much simpler.
Controllers and ActiveRecord alike still have many problem (controller actions "input" and "output" are both object state, ActiveRecord is a mix of domain and persistence functionality) — but this pattern can still vastly improve code without having to stray from common Rails conventsions, and the expectations of other code or programmers.
John Bachir – Co-Founder and CTO of Medstro