7
\$\begingroup\$

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 >= 5:
            fives = fives + 1
            currentvalue = currentvalue + 5
        if targetvalue - currentvalue < 5:
            if targetvalue - currentvalue >= 2:
                twos = twos + 1
                currentvalue = currentvalue + 2
            if targetvalue - currentvalue == 1:
                ones = ones + 1
                currentvalue = currentvalue + 1
            else:
                return fives, twos, ones
\$\endgroup\$
  • 1
    \$\begingroup\$ Maybe the modulo operator can help with the while True? \$\endgroup\$ – Tamoghna Chowdhury Mar 11 '17 at 10:36
  • \$\begingroup\$ @TamoghnaChowdhury thanks for responding! I haven't learnt the modulo operator, so I think it should be possible without (otherwise it wouldn't be a question on the course) \$\endgroup\$ – user133214 Mar 11 '17 at 10:38
  • \$\begingroup\$ My answer will feature the modulo (or remainder) operator, and it helps a lot with this stuff, so i suggest you learn about it. \$\endgroup\$ – Tamoghna Chowdhury Mar 11 '17 at 10:39
  • \$\begingroup\$ @TamoghnaChowdhury thank you! I certainly appreciate your help (by all means please show me how you would solve), but I will also see if someone can help me with the code as I tried to write it. \$\endgroup\$ – user133214 Mar 11 '17 at 10:45
2
\$\begingroup\$

The code is not returning the correct answer due to its breaking out of the loop early:

        if targetvalue - currentvalue == 1:
            ones = ones + 1
            currentvalue = currentvalue + 1
        else:
            return fives, twos, ones

The if statement can be reached when targetvalue - currentvalue is equal to 2. Because it is not equal to 1, the return statement is executed even though currentvalue is less than targetvalue. Removing the else condition should give the correct values.

That said, an implementation that adheres to the final sentence of the problem description might be clearer. First doing 5p, then 2p, and finally 1p. Here's how the original code can be reworked to do that:

def stamps(targetvalue):
    currentvalue = 0
    fives = 0
    twos = 0
    ones = 0

    while targetvalue - currentvalue >= 5:
        fives = fives + 1
        currentvalue = currentvalue + 5

    while targetvalue - currentvalue >= 2:
        twos = twos + 1
        currentvalue = currentvalue + 2

    while targetvalue - currentvalue >= 1:
        ones = ones + 1
        currentvalue = currentvalue + 1

    return fives, twos, ones

Limiting the loops to a few lines and using the same pattern for all values makes the code easier to follow and less error prone.

\$\endgroup\$
  • \$\begingroup\$ Hi, this answer was really helpful as it was helping me using the tools I was trying to use. The other answers, while it is good for me to learn new tools, ignored the method I was trying to follow. Will review a bit later but it looks really clear to me. Thank you! \$\endgroup\$ – user133214 Mar 13 '17 at 4:28
6
\$\begingroup\$

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

  1. 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.

  2. 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.

  3. while True: is a bad idea, as explained above.

  4. 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
\$\endgroup\$
  • \$\begingroup\$ Thanks Tamoghna, I do have a loose understanding of division and remainder in modulus, but it hasn't come up in the course I'm currently at, so trying to figure out what the course designers are expecting me to do... \$\endgroup\$ – user133214 Mar 11 '17 at 11:54
  • \$\begingroup\$ Much appreciate your input and tuition on this, by the way. \$\endgroup\$ – user133214 Mar 11 '17 at 11:54
3
\$\begingroup\$

Using builtin divmod():

def stamps(pence):
    fives, pence = divmod(pence, 5)
    twos, ones = divmod(pence, 2)
    return fives, twos, ones

Example:

>>> stamps(11)
(2, 0, 1)

that is you can get 11 pence using two 5p and one 1p stamps.

\$\endgroup\$
  • \$\begingroup\$ Divmod is a new one for me - will have to learn to use it! Thanks for responding. \$\endgroup\$ – user133214 Mar 13 '17 at 21:22
  • \$\begingroup\$ divmod is just a function for returning the quotient and remainder from an integer division at the same time. Using existing APIs to shorten line count is cool, though. \$\endgroup\$ – Tamoghna Chowdhury Mar 27 '17 at 7:13
  • \$\begingroup\$ @user133214 see above comment - it is just accomplishing the same thing as an explicit remainder calculation in half the line count. \$\endgroup\$ – Tamoghna Chowdhury Mar 27 '17 at 7:15
  • \$\begingroup\$ @TamoghnaChowdhury it is not about the line count. It is about using the right tool for the job. Readability counts. It is a builtin function (it means that it is important enough to be always available). For integers: divmod(a, b) == (a//b, a%b). Follow the link in the answer for more details. \$\endgroup\$ – jfs Mar 27 '17 at 7:31
  • \$\begingroup\$ However it gets justified, the side-effect of lower line-count is still appreciable. And forgive me if I disagree with you, but I consider readability (forgiving line count) to be equal in either the function or explicit approach. \$\endgroup\$ – Tamoghna Chowdhury Mar 27 '17 at 11:16

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.