I have big search method in my model that corresponds to search a proper Car.

It looks like this:

  def self.search(params) 
    cars = joins(:reservations).where.not("reservations.reception_time <= ? AND reservations.return_time >= ?", 
      params[:return_date], params[:handover_date])
    cars = joins(:car_class).where("car_classes.id= ?", params[:car_class])
    cars = cars_at_both_locations(params[:handover_location], params[:return_location])
    cars = params[:car_body_style] == [""] ? cars : joins(:car_configuration).
       where("car_configurations.body_style_id = ?", params[:car_body_style]) 
    cars = params[:car_fuel] == [""] ? cars : where(fuel: params[:car_fuel]) 
    cars = params[:car_transmission] == [""] ? cars : where(transmission: params[:car_transmission]) 
    cars = params [:car_seats] == [""] ? cars : car_seats(params[:car_seats])
    cars = Car.joins(:prices).where('prices.to_days >= ?', 
      (Date.parse(params[:return_date]) - Date.parse(params[:handover_date])).to_i)

It is very unreadable now.

Does anyone have an idea how to refactor this method to be more readable?


1 Answer 1


Some notes on your code:

  • As you know, in Rails, params usually refers to query strings in a request. In a model I'd use something like attributes or simply options to avoid confussion.
  • Variable cars reused multiple times: yes, it's a very usual pattern, but IMO it's not right. I think different values deserve different names.
  • You have a pattern to abstract here, use inject.

I gave an answer in SO a while ago with exactly this kind of problem, that's the approach I'd recommend: Approach post in SO


Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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