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.

I'm brand new to both Ruby and Rails, and while all the following works, it seems a bit messy.

This is on a classified model to generate WHERE/AND WHERE clauses based on whether or not the user supplies a value through a dropdown. I'm not sure if there's a more Active Recordy way of doing this:

class Classified < ActiveRecord::Base
   attr_accessible :car_id, :owner_id, :owner, :price, :condition, :description, :mileage
   belongs_to :car
   belongs_to :owner, :polymorphic => true
   has_one :model, :through => :car
   has_one :make, :through => :model
   has_many :pictures
 scope :filter_active_cars, lambda { |options| 
 filter = {}
 filter[:cars] = {:year => options[:year]} unless options[:year].empty?
 filter[:makes] = {:id => options[:make]} unless options[:make].empty?
 filter[:models] = {:id => options[:model]} unless options[:model].empty?

 joins(:car, :model, :make) 
 .where(filter)
 .includes(:car, :model, :make)
 }

The view has this

=select_tag('car[year]', options_for_select(years, :selected => @selected_year))
=collection_select(:car, :make, Make.all, :id, :name, {:include_blank => '- Make -', :selected => @selected_make}) 
=collection_select(:car, :model, @selected_make ? Model.where(:make_id => @selected_make) : Model.all, :id, :name, {:include_blank => '- Model -', :selected => @selected_model}) 

This is the way I'm passing the "select state" on the classified view. The controller has this snippet in the index action. This is so when a user filters a result and submits the form, the corresponding select box has the same value for the form helpers above.

if params[:car]
  params[:car].each do |option, value|
    next if value.empty?
    eval("@selected_#{option} = #{value}")
  end
end

On the form partial to generate and edit a classified. Obviously the chaining of &&s sucks, but it's the only way to get the default selection on new, edit, and when a user submits a new form that contains errors.

=select_tag('car[year]', options_for_select(years, :selected => @classified.car.nil? ? nil : @classified.car.year), :prompt => "- Year -")
=collection_select(:make, :id, Make.all, :id, :name, {:prompt => '- Make -', :selected => (@classified.car && @classified.car.make && @classified.car.make.try(:id))}) 
=collection_select(:car, :model_id, Model.all, :id, :name, {:include_blank => '- Model -', :selected => (@classified.car && @classified.car.model && @classified.car.model.try(:id))}) 
share|improve this question
    
I think you'd better split your question into several –  Alexey Aug 7 '12 at 21:54
add comment

1 Answer

Maybe you can convert this:

scope :filter_active_cars, lambda { |options| 
 filter = {}
 filter[:cars] = {:year => options[:year]} unless options[:year].empty?
 filter[:makes] = {:id => options[:make]} unless options[:make].empty?
 filter[:models] = {:id => options[:model]} unless options[:model].empty?

 joins(:car, :model, :make) 
 .where(filter)
 .includes(:car, :model, :make)
 }
}

to this:

scope :filter_active_cars, lambda do |options| 
 with_option(:car, 'cars.year', :year).
 with_option(:make, 'makes.id', :make).
 with_option(:model, 'models.id', :model)
end

scope :with_option, lambda do |assoc, column, option|
  (options[option].present? ?
    joins(assoc).where(column => options[option]) :
    self).
  includes(assoc)
end

You can reuse the with_option scope in other models

share|improve this answer
    
I would further just stop using scopes, and instead use methods. Scopes are pretty much a weird way to define a singleton method on a ActiveRecord-behaving class that MUST return an ActiveRecord::Relation & has infinite Arity. Better to have a real method with real Arity that can return anything. –  krainboltgreene Feb 20 '13 at 17:46
    
the fact that a scope MUST return a relation is the reason we use scopes. I'm not a big fan of those, but at least when you use one you know what to expect from it. As to @Alexey's solution, +1 - that's probably what i'll come up with ; one could go further and abstract this behavior with metaprogramming. This would make a nice macro like filter :active_cars, options: {cars: {year: :year}, makes: {make: :id}, models: {model: :id}}. The macro would reflect on associations, build necessary scopes and map params keys to columns. –  m_x Jun 4 '13 at 7:11
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.