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_discountenddef determine_discount  if @discounts.empty?    @discounts = 0  else    @discounts = @discounts.inject(:+)   end   endThis 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.
Calculating shopping cart discounts
ruby;e commerce
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 discountend