Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
share|improve this question
    
It looks like much of the code is double-spaced (has extra blank lines). Is that an accident of cut-and-paste? –  Wayne Conrad Jan 8 at 23:53
add comment

1 Answer

up vote 2 down vote accepted

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.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.