CODEWITHSUNDEEP
ruby-on-railsruby
prajeesh
prajeesh

Reputation: 2382

Ruby array reject elements based on condition

I am trying to reject array items based on multiple conditions.

The code is as follows

      def fetch_items
        items= line_items.reject(&driving?)
        if new_order_history_enabled?
          items = items.reject{ |li| li.expenses == 0 }
        end
        items
     end
     def driving?
        proc { |line_item| LineItemType.new(line_item, segment).drive? }
     end

Is there a one liner or a more cleaner way to write this?

Something like

items= line_items.reject { |li| li.driving? && ( new_order_history_enabled? && li.expenses == 0)}

Upvotes: 0

Views: 776

Answers (2)

Les Nightingill
Les Nightingill

Reputation: 6156

Personally I don't think a one-liner is always cleaner, especially when it's a long one-liner. The style that (to me) is cleaner, is to write:

  def fetch_items
    items= line_items.reject(&:driving?)
    items= items.reject(&:zero_expenses?) if new_order_history_enabled?
  end

  def driving?
    proc { |line_item| LineItemType.new(line_item, segment).drive? }
  end

# in the LineItem class, define the zero_expenses? method:
  def zero_expenses?
    expenses.zero?
  end

Upvotes: 1

PCurell
PCurell

Reputation: 111

items= line_items.reject { |li| li.driving? || (new_order_history_enabled? && li.expenses == 0)}

Since you want both to apply here, I think you should use || instead of &&

That way, you are actually doing what you describe in your method. (and you only iterate once over the array, which is cool :) )

Although, and this is stylistic preference. I would prefer to do:

items = line_items.reject do |li| 
  li.driving? || 
    (new_order_history_enabled? && li.expenses == 0)
end

since it might be clearer at a glance what we are doing

Upvotes: 3

Related Questions