Calculating change based on input

Question

I decided to make a program that calculates change, based on an input. My code is extremely long, and I would like to know how I can fix this.

def make_change(amount)
  values = []
  coins = []
  hash = {}
  if amount >= 50
    coins.push(:H)
    values.push(amount / 50)
    amount = amount % 50
    if amount % 50 >= 25
      coins.push(:Q)
      values.push(amount / 25)
      amount = amount % 25
      if amount % 25 >= 10
        coins.push(:D)
        values.push(amount / 10)
        amount = amount % 10
        if amount % 10 >= 5
          coins.push(:N)
          values.push(amount / 5)
          amount = amount % 5
          if amount % 5 >= 1
            coins.push(:P)
            values.push(amount / 1)
          end
        end
      end
    end
  elsif amount >= 25
    coins.push(:Q)
    values.push(amount / 25)
    amount = amount % 25
    if amount % 25 >= 10
      coins.push(:D)
      values.push(amount/10)
      amount = amount % 10
      if amount % 10 >= 5
        coins.push(:N)
        values.push(amount/5)
        amount = amount % 5
        if amount % 5 >= 1
          coins.push(:P)
          values.push(amount/1)
        end
      end
    end
  elsif amount >= 10  
    coins.push(:D)
    values.push(amount / 10)
    amount = amount % 10
    if amount % 10 >= 5
      coins.push(:N)
      values.push(amount / 5)
      amount = amount % 5
      if amount % 5 >= 1
        coins.push(:P)
        values.push(amount / 1)
      end
    end
  elsif amount >= 5
    coins.push(:N)
    values.push(amount / 5)
    amount = amount % 5
    if amount % 5 >= 1
      coins.push(:P)
      values.push(amount / 1)
    end
  elsif amount >= 1
    coins.push(:P)
    values.push(amount / 1)
  else
    coins = []
    values = []
  end
  Hash[coins.zip(values)]
end

Examples

make_change(42) should result in
=> {:Q => 1, :D => 1, :N => 1, :P => 2

make_change(91) should result in
=> {:H => 1, :Q => 1, :D => 1, :N => 1, :P => 1

Can you add some information, which input should result in which output? To calculates change I would expect two input values: Price and given amount. — knut
– knut, Commented Nov 5, 2014 at 20:38
@knut, I expect amount is the amount of change to provide. — Cary Swoveland
– Cary Swoveland, Commented Nov 5, 2014 at 22:20
Ok, I think I got your problem. You are talking about the US-$ with (H)alf-Dollar coin, (Q)uarter, (D)ime, (N)ickel and (P)ence. I tried your code with make_change(23) and get {:D=>2}. Is it an error and the :P=>3 is missing? — knut
– knut, Commented Nov 5, 2014 at 22:50
The key message from the answers is that you want a data-driven algorithm, where the set of coin denominations is a second input. — 200_success
– 200_success, Commented Nov 5, 2014 at 23:27

Cary Swoveland · Accepted Answer · 2014-11-06 04:54:11Z

Here's one way, that uses the method Fixnum#divmod to advantage:

COINS = [["H", 50], ["Q", 25], ["D", 10], ["N", 5], ["P", 1]]

def change(amount)
  raise ArgumentError, 'Only non-negative integers permitted' unless
    amount.is_a?(Fixnum) && amount >= 0
  return [] if amount.zero?
  COINS.map do |label, value|
    nbr, amount = amount.divmod(value)
    [nbr, label]
  end
end

change(260) #=> [[5, "H"], [0, "Q"], [1, "D"], [0, "N"], [0, "P"]]
change(95)  #=> [[1, "H"], [1, "Q"], [2, "D"], [0, "N"], [0, "P"]]
change(100) #=> [[2, "H"], [0, "Q"], [0, "D"], [0, "N"], [0, "P"]]
change(80)  #=> [[1, "H"], [1, "Q"], [0, "D"], [1, "N"], [0, "P"]]
change(45)  #=> [[0, "H"], [1, "Q"], [2, "D"], [0, "N"], [0, "P"]]
change(16)  #=> [[0, "H"], [0, "Q"], [1, "D"], [1, "N"], [1, "P"]]
change(1)   #=> [[0, "H"], [0, "Q"], [0, "D"], [0, "N"], [1, "P"]]
change(0)   #=> []
change(-3)  #=> ArgumentError: Only non-negative integers permitted...
change(1.2) #=> ArgumentError: Only non-negative integers permitted...
change('c') #=> ArgumentError: Only non-negative integers permitted...

Josiah · Accepted Answer · 2014-11-05 21:44:55Z

I would start by searching for ways to eliminate the deep nesting of the if statements and pulling the triad of similar commands you are repeating into a reusable function.

coins.push(:X)
values.push(amount/y)
return # some mutation imposed on amount

is an obvious template for a reusable function. Any time you copy-paste, you should start asking yourself how to avoid any more copy-paste behavior.

I'm not sure if I'm right about this, but it looks like this might be some sort of refining algorthm. You may want to replace all of those nested ifs with some sort of iterative calculation instead. You may be able to get your variable values for the contitional in the ifs by some function, but if not, you could put them in a list of truples and then map your calculation over that list. Something like

[(1,50,:H),(50,25,:Q),(25,10,:D),(10,5,:N),(5,1,:P)].each { |tuple| func(tuple.fst, tuple.snd, tuple.thrd) } 
# where func(fst, snd, thrd) is a generalization of 
if amount % fst >= snd
  coins.push(thrd)
  values.push(amount/snd)
  amount = amount % snd
end

#notice that this calculation mutates state.  You could avoid such mutation by using reduce instead of map to keep the state encapsulated in your iterative calculation over the list of parameters you want to pass in.  Refer to the Ruby docs for a starting place on how to do this.

Please treat this as an approach/idea presented in psuedo-code. I haven't tested this solution, and I also know it's not a comprehensive solution but only addresses the first part of the outer-most if block, but it's meant to point you in one possible direction for drying up your code.

The basic idea here is that you may make some progress in drying up your code simply by considering ways to organize your data into lists and/or structures that you can perform a calculation over, instead of thinking of your method in terms of a procedure. Extract the common behavior out into some smaller helper functions, then organize your data in a way that facilitates performing your calculation(s) over it. — Josiah
– Josiah, Commented Nov 5, 2014 at 21:56

knut · Accepted Answer · 2014-11-05 23:30:42Z

My solution:

#Define the coins with value
#(Remark: Ruby Hashs are ordered, so this can be used).
VALUES = {
  #'$' => 100,
  :H => 50,
  :Q => 25,
  :D => 10,
  :N =>  5,
  :P =>  1,
}
# In: Get money in cents
# Out: Change the minimum quantity of coins
def make_change(amount, values = VALUES)
  change = {}  
  values.each{|coin, value|
    num, amount = amount.divmod(value)
    change[coin] = num if num > 0
  }
  change
end

Test:

[23,100,42,91].each{|i|
 puts "%-4i: %-10s" % [ i, make_change(i)]
 }

result:

23  : {:D=>2, :P=>3}
100 : {:H=>2}   
42  : {:Q=>1, :D=>1, :N=>1, :P=>2}
91  : {:H=>1, :Q=>1, :D=>1, :N=>1, :P=>1}

One advantage of my solution: You can add new coins very easy. Just add '$' => 100, to support a one dollar coin. (The name of the coin must be uniq!).

Or just give your own coin definition as parameter, e.g. for Euros:

EUR_VALUES = {
  :'2EUR' => 200,
  :EUR => 100,
  50 => 50,
  20 => 20,
  10 => 10,
    5 =>  5,
    2 =>  2,
    1 =>  1,
}
[23,42,91,100,101].each{|i|
 puts "%-4iEUR-Cents: %-10s" % [ i, make_change(i, EUR_VALUES)]
 }

Result:

23  EUR-Cents: {20=>1, 2=>1, 1=>1}
42  EUR-Cents: {20=>2, 2=>1}
91  EUR-Cents: {50=>1, 20=>2, 1=>1}
100 EUR-Cents: {:EUR=>1} 
101 EUR-Cents: {:EUR=>1, 1=>1}

Just a note that, as you rely on the order of the elements of the hash VALUES, this requires Ruby 1.9+, but of course that's not necessarily a problem. It was for that reason, however, that I chose to use an array rather than a hash in my answer. — Cary Swoveland
– Cary Swoveland, Commented Nov 6, 2014 at 4:30
@CarySwoveland You are right. 2nd remark: the hash is ordered be insertion sequence, not any alphabetic order. — knut
– knut, Commented Nov 6, 2014 at 9:48

Stack Exchange Network

Calculating change based on input

3 Answers 3

You must log in to answer this question.

Linked

Hot Network Questions

Calculating change based on input

3 Answers 3

You must log in to answer this question.

Linked

Related

Hot Network Questions