Problem
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, 😀 => 1, :N => 1, 😛 => 2
make_change(91) should result in
=> {:H => 1, :Q => 1, 😀 => 1, :N => 1, 😛 => 1
Solution
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...
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.
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}