1
\$\begingroup\$

So a page where I do a search based on a bunch of filters but all of them are optionals. I have my controller here:

def distribution
    begin
      @service_request = ServiceRequest.find(params[:id])
      @claimed, @unclaimed = 0, 0
      @conditions = ContractorSearchConditions.new()
      @conditions.zipcode = @service_request.zipcode.to_s
      @conditions.search_terms = params[:filters].present? && params[:filters][:keywords].present? ? params[:filters][:keywords] : 'general'
      params[:filters].present? && params[:filters][:proximity].present? ? @conditions.mile_radius = params[:filters][:proximity] :
      params[:filters].present? && params[:filters][:results].present? ? @conditions.page_size = params[:filters][:results] :
      @conditions.page = 1
      @contractors = Contractor.search(@conditions)
      @contractors.each do |contractor|
        if contractor.uid == 26
          @unclaimed = @unclaimed+ 1
        else
          @claimed = @claimed+ 1
        end
      end
    rescue => e
      flash[:error] = "#{e}"
      redirect_to :action => :edit
    end
  end

And my struct here:

class ContractorSearchConditions < Struct.new(:search_terms, :state, :zipcode, :lat, :lng, :mile_radius, :account_type, :page, :page_size)

  #convert zipcode into lat, lon, and state
  def prepare
    zip = ZipCode.find(zipcode)
    self.state.nil? ? self.state = zip.state : self.state = self.state
    self.lat.nil? ? self.lat = zip.latitude.to_f : self.lat = self.lat.to_f
    self.lng.nil? ? self.lng = zip.longitude.to_f : self.lng = self.lng.to_f
    self.mile_radius.nil? ? self.mile_radius = 15 : self.mile_radius = self.mile_radius.to_i
    self.page_size.nil? ? self.page_size = 50 : self.page_size = self.page_size.to_i
    self.page.nil? ? self.page = 1 : self.page = self.page
  end


end

and

The thing is that my struct will be used in different controller across the app so I went to check the params and set default ones.

But I'm quite not happy with the all my line of if.

Any idea how I could make it better ?

Edit: New distribution method:

    begin
      @service_request = ServiceRequest.find(params[:id])
      if params[:filters].nil?
        params[:filters] =  Hash.new
        params[:filters] = {:mile_radius => 15, :page_size => 50, :search_terms => 'general', :score => 100}
     end
     @conditions = ContractorSearchConditions.new(params[:filters])
     @conditions.zipcode = @service_request.zipcode.to_s
     @contractors = Contractor.search(@conditions)
     @claimed, @unclaimed = 0, 0
     @contractors.each do |contractor|
      if contractor.uid == 26
        @unclaimed = @unclaimed+ 1
      else
        @claimed = @claimed+ 1
      end
    rescue => e
      flash[:error] = "#{e}"
      redirect_to :action => :edit
    end
  end

and here is the view:

<fieldset>
            <div class="row">
                <div class="form-group col-lg-12">
            <%= f.label :keywords, "Keywords", :class => 'control-label' %>    
          <%= f.input :keywords, :required => true, :label => false, :as => :string, :input_html => {:class => 'form-control', :maxlength => 255, :value => params[:filters][:keywords]}, :no_wrapper => true %>
                    <hr>
                </div>
            </div>
            <div class="row">
                <div class="form-group col-lg-2">
            <%= f.label :results, "# Results", :class => 'control-label' %>    
          <%= f.input :results, :required => true, :label => false, :input_html => {:class => 'form-control', :value => params[:filters][:results]}, :no_wrapper => true, :as => :number %>
                </div>
                <div class="form-group col-lg-2">
            <%= f.label :proximity, "Proximity", :class => 'control-label' %>    
          <%= f.input :proximity, :required => true, :label => false, :input_html => {:class => 'form-control', :value => params[:filters][:proximity]}, :no_wrapper => true, :as => :number %>
                </div>
            </div>
        </fieldset>

and my struct:

class ContractorSearchConditions < Struct.new(:search_terms, :state, :zipcode, :lat, :lon, :mile_radius, :account_type, :page, :page_size)

  #convert zipcode into lat, lon, and state
  def prepare
    zip = ZipCode.find(zipcode)
    self.lat = (lat.nil? ? zip.latitude : lat).to_f
    self.lon = (lon.nil? ? zip.longitude : lon).to_f
    self.state = (state.nil? ? zip.state : state).to_s
    self.mile_radius = (mile_radius.nil? ? '15' : mile_radius).to_i
    self.page_size = (page_size.nil? ? 50 : page_size).to_i
    self.page = (page.nil? ? 1 : page).to_i
    self.search_terms = (search_terms.nil? ? 'general' : search_terms).to_s
  end


end
\$\endgroup\$
5
\$\begingroup\$

Many things here.

Style considerations

First, in ruby conditionals are expressions, so instead of :

self.lat.nil? ? self.lat = zip.latitude.to_f : self.lat = self.lat.to_f

you can do :

self.lat = self.lat.nil? ? zip.latitude.to_f : self.lat.to_f

you can also get rid of self when not assigning :

self.lat = lat.nil? ? zip.latitude.to_f : lat.to_f

you can also group statements :

self.lat = (lat.nil? ? zip.latitude : lat).to_f

also, this is roughly equivalent to :

self.lat ||= zip_latitude
self.lat = lat.to_f

Design considerations

Your controller is too fat. especially this :

  @conditions = ContractorSearchConditions.new()
  @conditions.zipcode = @service_request.zipcode.to_s
  @conditions.search_terms = params[:filters].present? && params[:filters][:keywords].present? ? params[:filters][:keywords] : 'general'
  params[:filters].present? && params[:filters][:proximity].present? ? @conditions.mile_radius = params[:filters][:proximity] :
  params[:filters].present? && params[:filters][:results].present? ? @conditions.page_size = params[:filters][:results] :
  @conditions.page = 1

Basicly all of this should be in the body of ContractorSearchConditions#initialize method (give up the struct, use a real class : too much logic is attached to this entity to be a simple bag of data), so that you can do :

@conditions = ContractorSearchConditions.new( params[:search_conditions] )

To be continued I will come back when i have more time to analyze your logic, but here's a hint : when you have a lot of conditionals, it usually means you failed to capture a concept / mechanism as an object. Find out what concept(s) it is and you should be able to clean this.

EDIT : Refactoring

Let's analyze this bit of code :

  @conditions.search_terms = params[:filters].present? && params[:filters][:keywords].present? ? params[:filters][:keywords] : 'general'
  params[:filters].present? && params[:filters][:proximity].present? ? @conditions.mile_radius = params[:filters][:proximity] :
  params[:filters].present? && params[:filters][:results].present? ? @conditions.page_size = params[:filters][:results] :
  @conditions.page = 1

first, we will use "real ifs" and proper indentation to see clear in this mess :

  if params[:filters].present? && params[:filters][:keywords].present?
    @conditions.search_terms = params[:filters][:keywords]
  else 
    @conditions.search_terms = 'general' 
  end

  if params[:filters].present? && params[:filters][:proximity].present?
    @conditions.mile_radius = params[:filters][:proximity]
  end

  if params[:filters].present? && params[:filters][:results].present?
    @conditions.page_size = params[:filters][:results]
  else
    @conditions.page = 1
  end

Still ugly, but more clear. Something important now stands out... Do you see it ?

  if params[:filters].present?

    # side note : here we use "presence" from ActiveSupport, 
    # which returns the object itself if it is not blank, or nil.
    @conditions.search_terms = params[:filters][:keywords].presence
    @conditions.mile_radius  = params[:filters][:proximity].presence 
    @conditions.page_size    = params[:filters][:results].presence

  end

  @conditions.search_terms ||= 'general'
  @conditions.page = 1 unless @condition.page_size

The whole logic has, in fact, two purposes :

  • initialize the @conditions object using params[:filters]
  • set default values if necessary

All of this should be the responsibility of ContractorSearchConditions#initialize.

class ContractorSearchConditions

  # here are defaults values, all contained in a constant.
  # Thanks to this, everyone that looks at that class knows
  # what to expect as default values with this object.
  #
  DEFAULTS = {
      search_terms: :general,
      page:         1,
      page_size:    50,  # those values are hard to find
      miles_radius: 15   # in the code you posted... not anymore !
  }.freeze

  def initialize( params = {} )
    # the magic happen here. We have default values, 
    # but let the caller override them :
    options = DEFAULTS.merge( params || {} ) 
    filters = options.delete( :filters ){ {} }

    assign_attributes( options )
    filters.each{|name, value| apply_filter( name, value )}
  end

  # use the attributes accessor to assign all values.
  # the benefit here is that each accessor encapsulates
  # rules about what is a valid value to assign, how to coerce it, etc.
  # one could also easily filter which attributes can be assigned this way,
  # à la "attr_accessible"
  #
  def assign_attributes( attributes )
    attributes.each{|attr,value| public_send "#{attr}=", value }
  end

  # example of custom writer with safety nets all over the place :
  # 
  def page=( value )
    return @page if value.blank?
    @page = value.to_i
  rescue NoMethodError 
    raise ArgumentError, "invalid value '#{value}' for page"
  end

  # we can do something somewhat similar with filters :
  #
  def apply_filter( name, value )
    public_send "#{name}_filter", value
  rescue NoMethodError
    raise ArgumentError, "unknown filter '#{name}'"
  end

  # example of filter :
  #
  def proximity_filter( value )
    self.mile_radius = value
    # this seems dumb, but would be really useful if you have complex behavior
    # like multiparameter filters, etc.  
  end

end

What ? but this is far more complex !

yes... and no.

  • This implementation provides encapsulation of data and behavior, and ensures that your object always initializes with default, sensible values, in a consistent state. This is the heart of OOP!

  • This also means you won't have to repeat this code over and over if you need it in another controller : in other words, reusability.

  • this is a lot of logic, but you're doing many things ! In fact, if you want this to be pure, outrageously OO code, you would have to create one object for each responsibility (SRP) :

    • extract parameters from a hash
    • map them to a set of attributes
    • coerce all values and guard against meaningless ones
    • etc.
  • For now, you've stuffed a lot of logic in the controller. This works for small projects, but can quickly become a code swamp in bigger ones. The whole process you perform should have a dedicated place to live ! That's why many people (me included) would use some sort of ContractorSearch object that would represent... a search (duh) and that you would be able to manipulate like, say, an ActiveModel object. A (bit outdated) example of such approach can be seen in railscasts #111. This even allows you to easily save your searches !

  • i admit i have a tendency to overengineer. My implementation may be too much, but you get the spirit...

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ First of all thank you! I changed my method based on your comment. I also added the view. Here is the behaviour. The user can access to the page and then apply filter if the default results are not good enough. But I want to avoid logic in my view as much as possible, that's why I instantiate the params in the controller and I also want the inputs to be populated. What do you think ? \$\endgroup\$ – bl0b Sep 25 '13 at 17:36
  • \$\begingroup\$ i'm still editing my answer, I'll come back to you in a few minutes \$\endgroup\$ – m_x Sep 25 '13 at 17:42
  • \$\begingroup\$ wow, i may have gone over the board now. Sorry if its too long \$\endgroup\$ – m_x Sep 25 '13 at 17:58
  • \$\begingroup\$ as to your question, you should really see the railscasts #11 i linked in my answer. \$\endgroup\$ – m_x Sep 25 '13 at 18:04
  • \$\begingroup\$ one more word : with rails 3+, our apply_filter method can simply add scopes over a base relation. I usually have such a method, with 'optional' scopes that return an unchanged relation if a blank param is passed. This way you can chain them at will... \$\endgroup\$ – m_x Sep 25 '13 at 18:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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