Duplicate SQL code in controller

Question

How I can rewrite or refactor my controller code? I have the same SQL query (@plan_gp_users) in all defs.

 class PlanGpsController < ApplicationController
 def index
 @search = PlanGp.joins(:user).search(params[:q])
 @plan_gps = @search.result.order("created_at DESC").page(params[:page]).per(15)
 @plan_gp_users = User.where("users.ab_id = :abs_id AND users.id != :user_id AND is_admin IS NULL AND role != 'head'", {:abs_id => current_user.ab_id,:user_id =>current_user.id})
 respond_to do |format|
 format.js
 format.html # index.html.erb
 format.json { render json: @plan_gps }
 end
 end
 def show
 @plan_gp = PlanGp.find(params[:id])
 @plan_gp_users = User.where("users.ab_id = :abs_id AND users.id != :user_id AND is_admin IS NULL AND role != 'head'", {:abs_id => current_user.ab_id,:user_id =>current_user.id})
 respond_to do |format|
 format.js
 format.html # show.html.erb
 format.json { render json: @plan_gp }
 end
 end
 # GET /plan_gps/new
 # GET /plan_gps/new.json
 def new
 @plan_gp = PlanGp.new
 # 3.times { render @plan_gp.user_id }
 # .joins("LEFT JOIN plan_gps ON plan_gps.user_id = users.id and strftime('%Y-%m','now') = strftime('%Y-%m',plan_gps.month)") AND plan_gps.user_id is null 
 @plan_gp_users = User.where("users.ab_id = :abs_id AND users.id != :user_id AND is_admin IS NULL AND role != 'head'", {:abs_id => current_user.ab_id,:user_id =>current_user.id})
 # raise @plan_gp_users.to_sql
 respond_to do |format|
 format.js
 format.html # new.html.erb
 format.json { render json: @plan_gp }
 end
 end
 # GET /plan_gps/1/edit
 def edit
 @plan_gp = PlanGp.find(params[:id])
 @plan_gp_users = User.where("users.ab_id = :abs_id AND users.id != :user_id AND is_admin IS NULL AND role != 'head'", {:abs_id => current_user.ab_id,:user_id =>current_user.id})
 end
 # POST /plan_gps
 # POST /plan_gps.json
 def create
 @plan_gp = PlanGp.new(params[:plan_gp])
 @plan_gp_users = User.where("users.ab_id = :abs_id AND users.id != :user_id AND is_admin IS NULL AND role != 'head'", {:abs_id => current_user.ab_id,:user_id =>current_user.id})
 User.where("id IN (:user_ids) AND role != :role", {:user_ids => params[:plan_gp]["user_id"],:role =>'head'}).select("id").map do|m| 
 @plan_gp = PlanGp.new(params[:plan_gp])
 @plan_gp.user_id = m.id
 @plan_gp.abs_id = current_user.ab_id
 if @plan_gp.save 
 @plan_gp_save = true
 else 
 @plan_gp_save = false
 end
 end
 @plan_gp.abs_id = current_user.ab_id
 respond_to do |format|
 if @plan_gp_save
 format.js
 format.html { redirect_to plan_gps_url }
 format.json { render json: @plan_gp, status: :created, location: @plan_gp } 
 else
 format.js
 format.html { redirect_to plan_gps_url }
 format.json { render json: @plan_gp.errors, status: :unprocessable_entity } 
 end
 end
 end
 # PUT /plan_gps/1
 # PUT /plan_gps/1.json
 def update
 @plan_gp = PlanGp.find(params[:id])
 @plan_gp_users = User.where("users.ab_id = :abs_id AND users.id != :user_id AND is_admin IS NULL AND role != 'head'", {:abs_id => current_user.ab_id,:user_id =>current_user.id})
 respond_to do |format|
 if @plan_gp.update_attributes(params[:plan_gp])
 format.js
 format.html { redirect_to @plan_gp, notice: 'Plan gp was successfully updated.' }
 format.json { head :no_content }
 else
 format.js
 format.html { render action: "edit" }
 format.json { render json: @plan_gp.errors, status: :unprocessable_entity }
 end
 end
 end
 # DELETE /plan_gps/1
 # DELETE /plan_gps/1.json
 def destroy
 @plan_gp = PlanGp.find(params[:id])
 @plan_gp.destroy
 respond_to do |format|
 format.js
 format.html { redirect_to plan_gps_url }
 format.json { head :no_content }
 end
 end
end

• Wayne Conrad: It looks like much of the code is double-spaced (has extra blank lines). Is that an accident of cut-and-paste?

Answer 1

You should use a named Scope in your model

class User < ...
 scope :by_ab_without_user, lambda do |ab_id, id| 
 where(:ab_id => ab_id, :is_admin => null).
 where("id != :user_id AND role != 'head'", { :user_id => id })
 end
 ...
end

You can then use it from your controller with

User.by_ab_without_user(current_user.ab_id, current_user.id)

I'm not a big fan of writing another scope for each query which occurs in a controller though (which might end up only used once). That just increases the noise in the model. So you should strive for orthogonal scopes which can be combined easily. So for the above example you might do:

class User < ...
 scope :not_admin, where(:is_admin => null)
 scope :not_head, where("role != 'head'")
 scope :not_user, lambda { |id| where("id != ?", id) }
 scope :by_ab, lambda { |ab_id| where(:ab_id => ab_id) }
 ...
end

Then you could use them like this

User.not_admin.not_head.not_user(current_user).by_ab(current_user.ab_id)

Or if you need this particular combination often you can define a specialized scope with them (yes you can use other scopes in scope definitions).

But of course it depends on the problem domain wether or not you need that flexibility. So it's probably best to start with specialized scopes and orthogonalize later when you see fit.

Another thing you should do is to put common action initialization code in a before filter

class PlanGpsController < ApplicationController
 before_filter :init_plan_gp_users, :except => :destroy
 ...
private
 def init_plan_gp_users
 @plan_gp_users = ...
 end
end

But you still should use named scopes, because they improve code reusability across controllers.