0

I am doing this

    @orders = Order.unscoped.dated
    @orders = @orders.search_for(params["search"]) if params["search"].present?
    @orders = @orders.where(:items_received_status => true) if params[:air].present? && params[:air] == 't'
    @orders = @orders.tagged_with(params[:tags])            if params[:tags].present?

is there better way to write this same code

something like

@orders = Order do
  self.where(some condition)
  self.joins(some table)
end

there should be a way to avoid the repeating assignment operator

1
  • 1
    What don't you like about that code? Looks just fine to me. Jun 26, 2014 at 12:46

3 Answers 3

1

I think simplest way to do it is to create model methods for this:

class Order
  def self.air(air)
    return self.where(items_received_status: true) if air == 't'
    self
  end

  def search(search...

so you can chain it in controller like this:

Order.unscoped.dated.air(params[:air]).seach(...
0

You could do something like this. It's essentially the same code, but it's much neater because the logic isn't obfuscating what is being done:

@orders = build_orders

...

# In pure ruby, this is fine as a private method.  
# In rails, it would be better to have this method in the `Order` model, 
# perhaps split up as separate methods as Sergey Moiseev mentions.  

private

def build_orders
  orders = Order.unscoped.dated
  orders = orders.search_for(params["search"])          if params["search"].present?
  orders = orders.where(:items_received_status => true) if params[:air].present? && params[:air] == 't'
  orders = orders.tagged_with(params[:tags])            if params[:tags].present?

  orders
end
0
0

You can delegate the query to a BasicObject that proxies ActiveRecord::Relation.

class QueryDelegator < BasicObject
  def initialize(target)
    @target = target
  end

  def method_missing(*args, &block)
    result = target.send(*args, &block)
    @target = result if result.kind_of? @target.class
    result
  end
end

What's happening here is that you are proxying method calls to your Delegator to the ActiveRecord::Relation that is returned with the call to scope. Then, if the methods your proxy are returning ActiveRecord::Relations, or subclasses thereof, if will assign that result to the delegate target.

Then you would do:

@orders = QueryDelegator.new(Order.unscoped.dated)
@orders.search_for(params["search"]) if params["search"].present?
@orders.where(:items_received_status => true) if params[:air].present? && params[:air] == 't'
@orders.tagged_with(params[:tags])            if params[:tags].present?

Or you could refactor even further and do:

class Order < ActiveRecord::Base
  # ...
  def self.search(params)
    orders = query_builder(unscoped.dated)
    orders.search_for(params["search"]) if params["search"].present?
    orders.where(:items_received_status => true) if params[:air].present? && params[:air] == 't'
    orders.tagged_with(params[:tags])            if params[:tags].present?
  end

  def self.query_builder(scope = nil)
    scope ||= scoped
    QueryDelegator.new(scope)
  end
end

And then call your search in your controller like so:

Order.search(params)

Ryan Bates did a RailsCast on this pattern. Use at your own peril. As some commenters say, you may be sacrificing readability for only a little convenience.

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