5
\$\begingroup\$

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.

\$\endgroup\$
6
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ 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! \$\endgroup\$ – dax Dec 3 '13 at 20:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.