9
\$\begingroup\$

I wrote a function to calculate the change due from a transaction. It also prints the number of twenties, tens, fives, ones, and coins required to meet the amount. I am looking for suggestions for a more simplified and pythonic approach.

def change_return(cost,paid):

    change = round(paid-cost,2)
    print(f'Change due: {change}')

    change_dict = {}

    if change > 0:

        twenties = change // 20.00
        change_dict['Twenties'] = twenties
        change -= 20.00 * twenties

        tens = change // 10.00
        change_dict['Tens'] = tens
        change -= 10.00 * tens

        fives = change // 5.00
        change_dict['Fives'] = fives
        change -= 5.00 * fives

        ones = change // 1.00
        change_dict['Ones'] = ones
        change -= 1.00 * ones

        quarters = change // 0.25
        change_dict['Quarters'] = quarters
        change -= 0.25 * quarters

        dimes = change // 0.10
        change_dict['Dimes'] = dimes
        change -= 0.10 * dimes

        nickels = change // 0.05
        change_dict['Nickels'] = nickels
        change -= 0.05 * nickels

        pennies = change // 0.01
        change_dict['Pennies'] = pennies
        change -= 0.01 * pennies

    else:
        print('Insufficient funds')

    for key,value in change_dict.items():
        if value > 0:
            print(key + ': ' + str(value))
\$\endgroup\$
  • 3
    \$\begingroup\$ I don't have time for a full review, but wanted to mention that the general change making problem (use the smallest number of coins of provided denominations to make some sum) isn't correctly solved by always using the largest available coin. It works for standard US coins, but consider the case if some strange currency had 20, 9 and 1 and you want 37. Four 9s and a 1 is better than a 20, a 9 and eight 1s. \$\endgroup\$ – Josiah Sep 8 at 6:50
  • \$\begingroup\$ I don't know whether you care about hypothetical other currency or are just thinking about dollars at the moment. \$\endgroup\$ – Josiah Sep 8 at 6:51
10
\$\begingroup\$

Firstly, when working with monetary values, to avoid floating-point-number-related issues (you can try calling change_return(1, 1.15) in your solution to see it), it is more common to either (1) use cent as the unit and store all values as integers; or (2) use the Decimal class.

Secondly, the repetition in the code can be avoided. The currencies can be stored and iterated over one by one in a for loop. A simple approach to store all kinds of currencies is to use a list as others have shown. Another option is to use enumerations.

Thirdly, the logic could be simplified a bit by using the modulo operation change % denom to calculate change - change // denom * denom.

Following is refactored code:

from enum import Enum
from decimal import Decimal

class Currency(Enum):
    TWENTY = "20.00", "Twenties"
    TEN    = "10.00", "Tens"
    FIVE   =  "5.00", "Fives"
    ONE    =  "1.00", "Ones"
    # more ...
    PENNY  =  "0.01", "Pennies"

    def __init__(self, denomination, print_name):
        self.denomination = Decimal(denomination)
        self.print_name = print_name

def change_return(cost, paid):
    cost = Decimal(cost)   
    paid = Decimal(paid)

    change = paid - cost
    change_dict = {}  # Before Python 3.6 dictionaries do not preserve order so a sorting may be needed before printing

    if change < 0:
       return None   # Better to raise an exception if performance is not a major concern

    # Better to do rounding here rather than before calculating the change
    # round() could also be used here, quantize() offers more rounding options, if needed
    precision = Decimal("0.01")
    change = change.quantize(precision)

    # Note that the iteration order follows the declaration order in the Currency Enum,
    # therefore currencies should be declared in descending order of their denominations
    # to yield the desired outcome
    for cur in Currency:
        currency_cnt, change = divmod(change, cur.denomination)   # divmod(a, b) returns a tuple (a // b, a % b)
        if currency_cnt:  # Same as currency_cnt != 0
            change_dict[cur] = currency_cnt
    return change_dict

if __name__ == "__main__":
    changes = change_return("30", "86.13")
    if changes is None:
       print('Insufficient funds')
    else:
       for cur, count in changes.items():
           print(f"{cur.print_name:<9}: {count}")   # Format output using format strings
\$\endgroup\$
  • \$\begingroup\$ Good call out on Decimal or using ints and nice use of Enum. You could use a dict but you would have to sorted() the iterator unless you want to rely on Py3.6+ having ordered dictionaries. You don't really need the else after a return. I would be tempted to raise ValueError('Insufficient Funds') vs return None. \$\endgroup\$ – AChampion Sep 9 at 0:34
  • \$\begingroup\$ @AChampion Thanks for the comment. A few points: 1. It does rely on ordered dictionaries. A comment is added. I considered to return a list of tuples but eventually kept the dictionary just in case a lookup is needed on that. 2. I agree with you in this case but be aware that sometimes an explicit else can improve readablity. 3. I was also tempted to raise an error but eventually hold it back because of the potential performance overhead of handling exceptions and not knowing the full requirement. \$\endgroup\$ – GZ0 Sep 9 at 1:23
  • \$\begingroup\$ I'm loving the divmod use. Perhaps add a note about the fact that the iteration order of for cur in Currencyis dependent on the declaration order in the Currency Enum? This is a gotcha in progressing code development here. \$\endgroup\$ – Gloweye Sep 9 at 14:25
  • \$\begingroup\$ @JaccovanDorp Thanks. A comment about that is added. \$\endgroup\$ – GZ0 Sep 9 at 14:42
6
\$\begingroup\$

Style

I suggest you check PEP0008 https://www.python.org/dev/peps/pep-0008/ the official Python style guide to give you some insights on how to write a Pythonic style code.

def change_return(cost,paid):
  • Docstrings: Python Docstring is the documentation string which is string literal, and it occurs in the class, module, function or method definition, and it is written as a first statement. Docstrings are accessible from the doc attribute for any of the Python object and also with the built-in help() function can come in handy. You should include a docstrings to your functions explaining what they do and what they return:

    def calculate_total_change(cost, paid):
        """Calculate change and return total"""
        # code
    
  • f-Strings PEP 498 introduced a new string formatting mechanism known as Literal String Interpolation or more commonly as F-strings (because of the leading f character preceding the string literal). ... In Python source code, an f-string is a literal string, prefixed with 'f', which contains expressions inside braces and this facilitates the insertion of variables in strings.

    print(key + ': ' + str(value))
    

    can be written:

    print(f'{key}: {value}')
    
  • Blank lines: Too much blank lines in your function:

PEP008: Surround top-level function and class definitions with two blank lines. Method definitions inside a class are surrounded by a single blank line. Extra blank lines may be used (sparingly) to separate groups of related functions. Blank lines may be omitted between a bunch of related one-liners (e.g. a set of dummy implementations). Use blank lines in functions, sparingly, to indicate logical sections.

Code

  • A function should return a function usually returns something instead of printing.

    else:
            print('Insufficient funds')
    
        for key,value in change_dict.items():
            if value > 0:
                print(key + ': ' + str(value))
    

You might do the following instead:

return change_dict

Then use if __name__ == '__main__': guard at the end of your script which allows other modules to import your module without running the whole script like this:

if __name__ == '__main__': 
    change = change_return(15, 20)
    if change:
        for unit, value in change.items():
            print(f'{value}: {unit}')
    if not change:
        print('Amount paid is exact.')

Here's a refactored version of your code:

from fractions import Fraction


def calculate_total_change(cost, paid):
    """
    cost: a float/int representing the total cost.
    paid: a float/int representing amount paid
    return: Total change (dictionary).
    """
    units = [('Twenties', 20), ('Tens', 10), ('Fives', 5), ('Ones', 1), ('Quarters', Fraction(1 / 4)),
             ('Dimes', Fraction(1 / 10)), ('Nickels', Fraction(5 / 100)), ('Pennies', Fraction(1 / 100))]
    total_change = {unit[0]: 0 for unit in units}
    if cost > paid:
        raise ValueError(f'Insufficient amount {paid} for cost {cost}.')
    if cost == paid:
        print('No change.')
        return {}
    if cost < paid:
        change = paid - cost
        if change:
            for unit in units:
                while change - unit[1] >= 0:
                    total_change[unit[0]] += 1
                    change -= unit[1]
    return total_change


if __name__ == '__main__':
    change = calculate_total_change(15, 22.5)
    if change:
        for unit, value in change.items():
            print(f'{unit}: {value}')
    else:
        print('Amount exact')
\$\endgroup\$
  • 1
    \$\begingroup\$ Is there a reason you use Fraction over Decimal? Also, your return value is inconsistent - if someone pays the exact amount you return an integer instead of a dict, which will break your loop in if __name__ == "__main__". I think it would be more consistent to return an empty dict there. \$\endgroup\$ – Gloweye Sep 10 at 6:48
  • \$\begingroup\$ thanks for pointing this out, I'll edit the code and there is no specific reason for choosing Fraction over Decimal, I guess both would do the same job, I'm more familiar with Fraction that's all and why returning a zero will break the loop? \$\endgroup\$ – user203258 Sep 10 at 6:52
  • \$\begingroup\$ I think returning a zero would be inconsistent with what is indicated in the docstring however, it won't break the loop because both a zero and the empty dict have the same boolean value (False) \$\endgroup\$ – user203258 Sep 10 at 7:00
  • \$\begingroup\$ Oh, right, didn't see that if change:. However, perhaps someone will use the function somewhere else - and having to check it would be a pain in that case. \$\endgroup\$ – Gloweye Sep 10 at 7:14
5
\$\begingroup\$

I would clean it up by separating my declarations from my code. With the below code, it becomes immediately clear to the reader that you are working with the various denominations of US currency. I have to work to figure it out in your code. Also, this lends itself to using other currency. I could define a different denominations variable to work with different currencies without changing my code at all.

from collections import namedtuple

Denomination = namedtuple('Denomination', 'name value')
denominations = [
    Denomination("Twenties", 20.00),
    Denomination("Tens", 10.00),
    Denomination("Fives", 5.00),
    Denomination("Ones", 1.00),
    Denomination("Quarters", 0.25),
    Denomination("Dimes", 0.10),
    Denomination("Nickles", 0.05),
    Denomination("Pennies", 0.01)
]


def change_return(cost, paid):
    change = round(paid-cost,2)
    print(f'Change due: {change}')

    if change > 0:
        for d in denominations:
            used = change // d.value
            if used > 0:
                print(d.name + ": " + str(d.value))
            change -= d.value * used

    else:
        print('Insufficient funds')


if __name__ == '__main__':
    change_return(30, 75.13)
\$\endgroup\$
  • 5
    \$\begingroup\$ The poor person that gives exactly the right amount to the cashier/program. They are told that they have insufficient funds (it should probably be elif change < 0). \$\endgroup\$ – Graipher Sep 8 at 15:32
5
\$\begingroup\$

You have a lot of repeating code (x = change // y). To fix this, you can use the following function:


def get_denomination(change, denom):
    num_of_denom = change // denom
    return (num_of_denom, change - (num_of_denom * denom))
# New use-case
twenties, change = get_denomination(change, 20.00)

And, to avoid calculating the amount of every denomination in the case that change reaches zero before the end of your conditional block:


def change_return(cost,paid):

    change = round(paid-cost,2)
    print(f'Change due: {change}')

    change_dict = {}
    denominations = [20, 10, 5, 1, 0.25, 0.1, 0.05, 0.01]
    titles = ['Twenties', 'Tens', 'Fives', 'Ones', 'Quarters', 'Dimes',
              'Nickels', 'Pennies']

    for index, denomination in enumerate(denominations):
        num_denom, change = get_denomination(change, denomination)
        change_dict[titles[index]] = num_denom
        if change == 0:
            break

    if change < 0:
        print('Insufficient funds')

for key,value in change_dict.items():
    if value > 0:
        print(key + ': ' + str(value))
\$\endgroup\$
  • \$\begingroup\$ Why are you re-implementing the divmod() builtin? Also, it will be a bit better if you zip(titles, denominations) instead of enumerate + indexing. \$\endgroup\$ – Gloweye Sep 10 at 6:53

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.