I have a controller method for searching and listing invoices, sales orders and homes based on params name, how to simplify this index method`

def index
      @so = QbwcSalesOrder.paginate(:page => params[:page], :per_page => 30)
      if params[:qs] == "Sales Orders"
        @so = @so.where("ref_number = ? ",params[:keyword]) if params[:keyword].present?
   elsif params[:qs] == "Invoices" 
        @so = QbwcInvoice.paginate(:page => params[:page], :per_page => 30)
        @so = @so.where("ref_number = ? ",params[:keyword])  if params[:keyword].present?
   elsif params[:qs] == "All Homes" 
        @so = QbwcHome.paginate(:page => params[:page], :per_page => 30)
   elsif params[:qs] == "Existing Homes" 
        @so = QbwcHome.where(record_type: "FromQB").paginate(:page => params[:page], :per_page => 30)
   end
 end

CodePudding user response：

First of all unify hash syntax and indentation, it's hard to read.

Second paginate once at the end of the index method, looks like you paginate always the same way.

Third think about scopes or query object https://mkdev.me/en/posts/how-to-use-query-objects-to-refactor-rails-sql-queries

Here is simplified version, you can use it in index or move to the query object:

def index
  @so = QbwcSalesOrder.all

  @so = @so.where("ref_number = ? ", params[:keyword]) if params[:keyword] && params[:qs] == "Sales Orders"
  @so = @so.where("ref_number = ? ", params[:keyword]) if params[:keyword] && params[:qs] == "Invoices"
  @so = @so.where(record_type: "FromQB") if params[:qs] == "Existing Homes" 
  @so = @so if params[:qs] == "All Homes" 

  @so = @so.paginate(page: params[:page], per_page: 30)
end

CodePudding user response：

To me this looks like the method is doing way to much and needs to be split into managable parts:

def index
  @so = qs_model_scope(params[:qs])
          .paginate(page: params[:page], per_page: 30)
          .then do |scope|
            filter_by_keyword(scope)
          end
end

# ...

private

def qs_model_scope(qs)
  case qs
  when "Invoices":
    QbwcInvoice.all
  when "All Homes":
    # Smelly - should be a model scope
    QbwcHome.where(record_type: "FromQB")
  else
    QbwcSalesOrder.all
  end
end 

def filter_by_keyword(scope)
  if params[:keyword].present? && ["QbwcSalesOrder","QbwcInvoice"].include?(scope.model.name)
    # no need to use a SQL string
    scope.where(ref_number: params[:keyword])   
  else
    scope
  end
end

If the complexity of this continues to grow you should consider moving this filtering process out of the controller and into a separate object (such as a form object or service object or just a PORO) so that it can be tested in isolation.

CodePudding user response：

I'd split the logic into separate methods. As already mentioned in another answer it makes sense to move it to a query object or something.

def index
  scope = invoices || all_homes || existing_homes || sales_orders
  @so = scope.paginate(:page => params[:page], :per_page => 30)
end

private

def invoices
  return if params[:qs] != 'Invoices'
  return QbwcInvoice if params[:keyword].blank?
  QbwcInvoice.where(ref_number: params[:keyword])
end

def all_homes
  QbwcHome if params[:qs] == 'All Homes'
end

def existing_homes
  QbwcHome.where(record_type: 'FromQB') if params[:qs] == 'Existing Homes'
end

def sales_orders
  if params[:qs] == 'Sales Orders' && params[:keyword].present?
    QbwcSalesOrder.where(ref_number: params[:keyword])
  else
    QbwcSalesOrder
  end
end