Rails app query form code behind

Question

This is code of a rails app. The code has the logic used in a search form. It queries a MySQL database. Any thoughts on something that would make this unsafe or violated a rails principle?

/app/models/query.rb

class Query < ActiveRecord::Base
  attr_accessible :customer_email, :customer_first_name, :customer_last_name, 
                  :max_total_amount, :min_total_amount, :order_id, :order_is_deleted,
                  :order_max_date, :order_min_date, :results_limit, :postal_code,
                  :subscribes_to_daily_emails

 def orders
   @orders ||= find_orders
 end

  #TO DO:
  #1. Pagination:
  #2. Add limit if not present when there could be too many results. DONE! Set limit to 50.
  #3. Add export feature and remove limit for export.

  private
  def find_orders

    @results_limitVar = nil

    if results_limit.nil? == true
      @results_limitVar = 50
    else
      @results_limitVar = results_limit    
    end

    @subscribes_to_daily_emailsVar = nil

    if subscribes_to_daily_emails.nil? == false
      if subscribes_to_daily_emails 
        @subscribes_to_daily_emailsVar = 1
      else 
        @subscribes_to_daily_emailsVar = 0
      end
    end


    @order_is_deletedVar = nil

    if order_is_deleted.nil? == false
      if order_is_deleted 
        @order_is_deletedVar = 1
      else 
        @order_is_deletedVar = 0
      end
    end

    #Added orders explicitly because otherwise first_name, etc is ambiguous 
    #since it is in multiple models.

    orders = Order.order(:order_id)
    orders = orders.where("orders.first_name LIKE ?", "%#{customer_first_name}%")     if customer_first_name.present?
    orders = orders.where("orders.last_name LIKE ?", "%#{customer_last_name}%") if customer_last_name.present?    
    orders = orders.where("orders.email_address LIKE ?", "%#{customer_email}%") if customer_email.present?        
    orders = orders.where("orders.order_id like ?", "%#{order_id}%") if order_id.present?
    orders = orders.where("orders.is_deleted = ?", "#{@order_is_deletedVar}") if !@order_is_deletedVar.nil?
    orders = orders.where("total_amount >= ?", min_total_amount) if min_total_amount.present?
    orders = orders.where("total_amount <= ?", max_total_amount) if max_total_amount.present?
    orders = orders.where("order_date >= ?", order_min_date) if order_min_date.present?
    orders = orders.where("order_date <= ?", order_max_date) if order_max_date.present?
    orders = orders.joins("INNER JOIN customers ON customers.customer_id = orders.customer_id AND customers.postal_code LIKE ", "'%#{postal_code}%'") if postal_code.present?
    orders = orders.joins("INNER JOIN email_preferences ON email_preferences.customer_id = orders.customer_id AND email_preferences.subscribes_to_daily_emails = ", "#{@subscribes_to_daily_emailsVar}") if !@subscribes_to_daily_emailsVar.nil?  
    orders = orders.limit(@results_limitVar) if !@results_limitVar.nil? 
    orders
  end

end

Answer 1

I think you could change your code to this :

class Query < ActiveRecord::Base
  attr_accessible :customer_email, :customer_first_name, :customer_last_name, 
                  :max_total_amount, :min_total_amount, :order_id, :order_is_deleted,
                  :order_max_date, :order_min_date, :results_limit, :postal_code,
                  :subscribes_to_daily_emails

 def orders
   @orders ||= find_orders
 end

  #TO DO:
  #1. Pagination:
  #2. Add limit if not present when there could be too many results. DONE! Set limit to 50.
  #3. Add export feature and remove limit for export.

  private
  def find_orders
    @results_limitVar = results_limit || 50

    @subscribes_to_daily_emailsVar = nil

    if subscribes_to_daily_emails
      @subscribes_to_daily_emailsVar = subscribes_to_daily_emails ? 1 : 0
    end

    @order_is_deletedVar = nil

    if order_is_deleted
      @order_is_deletedVar = order_is_deleted ? 1 : 0
    end

    #Added orders explicitly because otherwise first_name, etc is ambiguous 
    #since it is in multiple models.

    orders = Order.order(:order_id)
    orders = orders.where("orders.first_name LIKE ?", "%#{customer_first_name}%")     if customer_first_name
    orders = orders.where("orders.last_name LIKE ?", "%#{customer_last_name}%") if customer_last_name
    orders = orders.where("orders.email_address LIKE ?", "%#{customer_email}%") if customer_email     
    orders = orders.where("orders.order_id like ?", "%#{order_id}%") if order_id
    orders = orders.where("orders.is_deleted = ?", "#{@order_is_deletedVar}") if @order_is_deletedVar.present?
    orders = orders.where("total_amount >= ?", min_total_amount) if min_total_amount
    orders = orders.where("total_amount <= ?", max_total_amount) if max_total_amount
    orders = orders.where("order_date >= ?", order_min_date) if order_min_date
    orders = orders.where("order_date <= ?", order_max_date) if order_max_date
    orders = orders.joins("INNER JOIN customers ON customers.customer_id = orders.customer_id AND customers.postal_code LIKE ", "'%#{postal_code}%'") if postal_code
    orders = orders.joins("INNER JOIN email_preferences ON email_preferences.customer_id = orders.customer_id AND email_preferences.subscribes_to_daily_emails = ", "#{@subscribes_to_daily_emailsVar}") if @subscribes_to_daily_emailsVar.present?
    orders = orders.limit(@results_limitVar)
    orders
  end

end

I refactored your if conditions. Instead of :

if var.nil? == false
if !var.nil?

It's better to write like this:

if var.present?

In your case you can simply do :

if var

For your query, please watch look at the scopes.

You can do this :

class Query < ActiveRecord::Base
  scope :first_name_like, lambda { |first_name| where("orders.first_name LIKE ?", "%#{first_name}%") }
  ...
  def find_orders
    ...
    orders = orders.first_name_like(customer_last_name)
    ...
  end
end

It's interessent for your loooooong inner join.

I think if you column is a boolean you don't need to convert the value and you could do this :

orders.where("orders.is_deleted = ?", "#{order_is_deleted}")

The last thinks. Instead of this :

@results_limitVar

Write like this (by convention) :

@results_limit_var