Calculating shopping cart discounts 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 &amp;&amp; attr[:quantity] &gt;= special.quantity
        @discounts &lt;&lt; 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.
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