CODEWITHSUNDEEP
sqlruby-on-railsrubyactiverecordruby-on-rails-4
Irakli Lekishvili
Irakli Lekishvili

Reputation: 34158

Rails ActiveRecord where clause

I want to select Cars from database with where clause looking for best DRY approach for my issue.

for example I have this two parameters 

params[:car_model_id] (int)
params[:transmission_id] (int)
params[:from_date]
params[:to_date]

but I dont know which one will be null

if params[:car_model_id].nil? && !params[:transmission_id].nil?
    if params[:from_date].nil? && params[:from_date].nil?
        return Car.where(:transmission_id => params[:transmission_id])
    else
        return Car.where(:transmission_id => params[:transmission_id], :date => params[:from_date]..params[:to_date])
    end
elseif !params[:car_model_id].nil? && params[:transmission_id].nil?
    if params[:from_date].nil? && params[:from_date].nil?
        return Car.where(:car_model_id=> params[:car_model_id])
    else
        return Car.where(:car_model_id=> params[:car_model_id], :date => params[:from_date]..params[:to_date])
    end
else
   return Car.where(:car_model_id=> params[:car_model_id], :transmission_id => params[:transmission_id], :date => params[:from_date]..params[:to_date])
end

what is best approach to avoid such bad code and check if parameter is nil inline(in where)

Upvotes: 4

Views: 1043

Answers (3)

Rodrigo
Rodrigo

Reputation: 4802

First, I would change this code to Car model, and I think there is no need to check if params doesn't exists.

# using Rails 4 methods
class Car < ActiveRecord::Base

  def self.find_by_transmission_id_or_model_id(trasmission_id, model_id)
     if transmission_id
       find_by trasmission_id: trasmission_id
     elsif model_id
       find_by model_id: model_id
     end
  end
end

In controller:

def action
   car = Car.find_by_transmission_id_or_model_id params[:trasmission_id], params[:car_model_id]
end

edit:

This code is fine while you have only two parameters. For many conditional parameters, look at ransack gem.

Upvotes: 0

Surya
Surya

Reputation: 15992

You can do:

car_params = params.slice(:car_model_id, :transmission_id).reject{|k, v| v.nil? }

and then:

Car.where(car_params)

Explanation: Since, you're checking if the particular key i.e.: :car_model_id and transmission_id exists in params. The above code would be something like this when you have just :transimission_id in params:

Car.where(:transmission_id => '1')

or this when you have :car_model_id in params:

Car.where(:car_model_id => '3')

or this when you'll have both: 

Car.where(:transmission_id => '1', :car_model_id => '3')

NOTE: This will work only when you have params keys as the column names for which you're trying to run queries for. If you intend to have a different key in params which doesn't match with the column name then I'd suggest you change it's key to the column name in controller itself before slice.

UPDATE: Since, OP has edited his question and introduced more if.. else conditions now. One way to go about solving that and to always keep one thing in mind is to have your user_params correct values for which you want to run your queries on the model class, here it's Car. So, in this case:

car_params = params.slice(:car_model_id, :transmission_id).reject{|k, v| v.nil? }
if params[:from_date].present? && params[:from_date].present?
  car_params.merge!(date: params[:from_date]..params[:to_date])
end

and then: 

Car.where(car_params)

Upvotes: 4

d.danailov
d.danailov

Reputation: 9800

what is best approach to avoid such bad code and check if parameter is nil inline(in where)

Good Question !

I will make implementation with two extra boolean variables (transmission_id_is_valid and car_model_id_is_valid)

transmission_id_is_valid = params[:car_model_id].nil? && !params[:transmission_id].nil?
car_model_id_is_valid = !params[:car_model_id].nil? && params[:transmission_id].nil?

if transmission_id_is_valid
   return Car.where(:transmission_id => params[:transmission_id])
elseif car_model_id_is_valid
   return Car.where(:car_model_id=> params[:car_model_id])
....
end

I think now is more human readable.

Upvotes: 0

Related Questions