Calculating shopping cart discounts

Question

I have a method that checks to see if a hash of given items should have discounts applied and if so, determines and returns the discount:

def get_discounts
  @items.each do |name, attr|
    @specials.each do |special|
      while name == special.sale_item && attr[:quantity] >= special.quantity
        @discounts << special.discount
        attr[:quantity] = attr[:quantity] - special.quantity
      end  
    end  
  end
  determine_discount
end

def determine_discount
  if @discounts.empty?
    @discounts = 0
  else
    @discounts = @discounts.inject(:+) 
  end   
end

This works perfectly, but is there a more concise way to write it? I'm looking especially at the two each loops. I'm also a bit iffy about the while loop - it was an if statement (if name == special.sale_item) but it felt like too much so I combined it into the while loop.

Answer 1

I agree with both of your suspected issues:

• Pointless iteration through @items hash: As you suspected, iterating through all entries of a hash defeats the purpose of a hash. That takes O(I * S) time — the number of items times the number of specials. Why not iterate through @specials, then look up the item by name? That's only O(S).
• while loop: The loop can be replaced with arithmetic.

In addition, I would like to point out some more problems:

• Surprising side-effect in getter: By convention, any method named get_*() is assumed to have no side-effects. However, get_discounts() alters @items, reducing their quantity. If that is intentional, you should rename the method to apply_discounts() or something even more suggestive that there is a side-effect.
• Abuse of instance variable: @discounts is assumed to be pre-initialized to an empty array before get_discounts() is called. get_discounts() populates the array, then calls determine_discount(), which converts it into a scalar. That indicates that @discounts is not storing the state of the object, so using an instance variable for that purpose is abuse.

Here is a revised version of the code:

def get_discounts
  discount = 0
  @specials.each do |special|
    if item = @items[special.sale_item]
      multiples = (item[:quantity] / special.quantity).floor
      discount += multiples * special.discount

      # Suspicious side-effect...
      # item[:quantity] -= multiples * special.quantity
    end
  end
  return discount
end