# 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 && 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.

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

• great answer, thank you - actually i had remedied the 'side-effect' but thanks for pointing that out. I did count = info[:quantity] and then count = count - special.quantity but your way is nicer. Cheers, really appreciate the push in the right direction! – dax Dec 3 '13 at 20:19