I see a lot of the following pattern in Rails projects:
class ProductsController < ApplicationController before_action :assign_post, only: %i(show edit) def index @posts = Post.paginate(params[:page]) end def show end def edit end private def assign_post @post = Post.find(params[:id]) end end
Most people do this, because for them
@post = Post.find(params[:id]) is duplication. Having
assign_post is more DRY.
I don't buy the duplication argument. The definition for DRY is:
“Every piece of knowledge must have a single, unambiguous, authoritative representation within a system."
Unfortunately, most people confused it with - "I don't want to have the same sequence of characters more than once". David Chelimsky have an excellent talk on that subject.
But, my biggest issue with the approach is that it "hides" all variables passed to the view. Especially if there are
except passed to
before_action. Then you have to become an interpreter to figure out what gets into the view. I can recall several situations when during a code review I found code passing unwanted variables to a view.
I prefer to think the following, when I read a controller:
@commentsto the view
showis called then
assign_postsis called before it, which sets
showis not included in this other action filter, but is not excluded for this third one...
Also, it is easier for a developer to spot, that too many variables are passed to the view if all of them are listed together.
Because of that, I prefer to use the following pattern:
class ProductsController < ApplicationController def show @post = find_post end def edit @post = find_post end private def find_post Post.find(params[:id]) end end
In this way, the duplication is moved into finder methods. So if I have to add a scope like
Post.find, I just need to apply it in