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."
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