Procedure to determine minimum number of stamps to make up a given monetary value I'm following the Udacity intro to CompSci, and I don't know why my answer isn't quite correct:

Question:


  Define a procedure, stamps, which takes as its input a positive
  integer in pence and returns the number of 5p, 2p and 1p stamps (p is
  pence) required to make up that value. The return value should be a
  tuple of three numbers (that is, your return statement should be
  followed by the number of 5p, the number of 2p, and the nuber of 1p
  stamps).
  
  Your answer should use as few total stamps as possible by first using
  as many 5p stamps as possible, then 2 pence stamps and finally 1p
  stamps as needed to make up the total.


My attempt

def stamps(targetvalue):
    currentvalue = 0
    fives = 0
    twos = 0
    ones = 0
    while True:
        if targetvalue == currentvalue:
            return fives, twos, ones
        if targetvalue - currentvalue &gt;= 5:
            fives = fives + 1
            currentvalue = currentvalue + 5
        if targetvalue - currentvalue &lt; 5:
            if targetvalue - currentvalue &gt;= 2:
                twos = twos + 1
                currentvalue = currentvalue + 2
            if targetvalue - currentvalue == 1:
                ones = ones + 1
                currentvalue = currentvalue + 1
            else:
                return fives, twos, ones
Let me start by saying that a while True: loop with a return embedded is a bad idea. It works, yes, but the train of logic becomes more complicated to follow the more branches are within the loop. Now if you had, say, 10p, 20p &amp; 50p coins, this would become more and more complicated until it reached a point where you would want to rewrite it in a more sensible way.

With that out of the way, let's get on with the review.


Hard-code as little as possible. Try to use more parameters. All those 5's are for a specific use-case. Try accepting them as parameters in your function.
Return a dict instead of a tuple - it associates each price with a count of stamps of that price and makes the returned value more general with respect to prices.
while True: is a bad idea, as explained above.
Your code style and formatting is OK as per the PEP8 style guide for Python.


The way you do it - you are reimplementing division and remainder from scratch with a loop. This is, well, useless.

The way you're doing it right now is OK if you can't use the above (course restrictions?), which would have helped a lot in this case - you could have used it to get rid of the while loop altogether, just looping over the allowed prices.

What can be done for your code is moving out the part which reimplements division and remainder into functions and using them, but then there remains no difference between the way I do it and the way I suggest for you to do it here.

(Note that the extraction into functions is necessary for reducing code duplication and extending your code to support more prices).

Here is my take:

def stamps(targetValue, prices):
    currentValue = targetValue
    sortedPrices = list(reversed(sorted(prices)))
    stampCounts = []
    for price in sortedPrices:
        stampCounts.append(currentValue // price) # note floor division
        currentValue = currentValue % price # remainder
    return dict(zip(sortedPrices, stampCounts)) # return a dict for caller's convenience