7
\$\begingroup\$

I have 3 types of coins: Gold, Silver, Copper.
1 silver = 100 copper.
1 gold = 100 silver.

My input is always in coppers, and I want to be able to make it a bit more readable. So far my code is:

def api_wallet_translate_gold(value):
    """Translate a value into string of money"""
    if value >= 10000:  # Gold
        return ("{0} gold, {1} silver and {2} copper."
                .format(str(value)[:-4], str(value)[-4:-2], str(value)[-2:]))
    elif value >= 100:  # Silver
        return "{0} silver and {1} copper.".format(str(value)[-4:-2], str(value)[-2:])
    else:  # Copper
        return "{0} copper.".format(str(value)[-2:])

It works, but I am wondering how could it be improved. I think there was a way to format it like {xx:2:2} or something but I can't remember how to do it.

Note: We never know how many gold digits we have, it could be 999999 to 1

\$\endgroup\$
10
\$\begingroup\$

It may less fragile if you deal with the numbers directly rather than converting to strings. It will also be cleaner code.

You could start with your values in a list sorted highest to lowest. Then in your function you can find the next-largest value and remained with divmod(). After than it's a matter of deciding how you want to format the resulting dict:

coins = [
    ("gold",  100 * 100),
    ("silver", 100), 
    ("copper", 1)
]

def translate_coins(value, coins):
    res = {}
    for coin, v in coins:
        res[coin], value = divmod(value, v)
    return res 

translate_coins(1013323, coins)

Result:

{'gold': 101, 'silver': 33, 'copper': 23}
\$\endgroup\$
  • \$\begingroup\$ This is almost perfect @MarkM, though the def translate_coins(value, coins): line combined with res[coin], value = divmod(value, v), could have the values (one of'em) renamed for easier reading... Neat trick with the dictionary assignment there... One question too, why not use a dict for coins instead of a list of tuples?... Then for coin, quantity in coins.items(): could be used with similar effect and one less object within another. That all said I think your answer is solid, just had a few nits to be picked that I could see. \$\endgroup\$ – S0AndS0 Apr 24 at 17:08
  • 2
    \$\begingroup\$ Thanks for the comment @S0AndS0 those a re great suggestions. I didn't use a dictionary for the coin values because this depends on doing the division in order from highest to lowest. It's only recently that you can count on the order of python dictionaries. \$\endgroup\$ – MarkM Apr 24 at 17:13
  • \$\begingroup\$ After further testing I see what you're doing, clever @MarkM, really clever there with reassigning value... though I don't envy debugging such mutations, it does totally make sense in this context to mutate... I also now see your wisdom in using a list of tuples, and retract my previous question in regards to using a dict as input to the translate_coins function; that would have made code far hairier than needed... Consider me impressed, eleven lines of code and you've taught me plenty new perversions with Python. \$\endgroup\$ – S0AndS0 Apr 24 at 17:46
3
\$\begingroup\$

That was a nice little break from work, tks for asking this question :-) I think this is a good use case for an object/class vs. a method.

I would create a Currency class, which then allows you to either print it, or access its attributes independently...

class Currency(object):

    COPPER_CONVERSION_MAP = {
        'copper': 1,
        'gold': 100 * 100,
        'silver': 100
    }
    gold = 0
    silver = 0
    copper = 0

    def __init__(self, copper=0, silver=0, gold=0):
        # convert all inputs into copper
        self.copper = (
            copper +
            silver * self.COPPER_CONVERSION_MAP['silver'] +
            gold * self.COPPER_CONVERSION_MAP['gold']
          )
        self.break_currency()

    def break_currency(self):
        for coin_type in ['gold', 'silver']:
            coins, coppers = divmod(self.copper, self.COPPER_CONVERSION_MAP[coin_type])
            setattr(self, coin_type, coins)
            self.copper = coppers

    def __str__(self):
        return '{:,} gold, {:,} silver and {:,} copper'.format(self.gold, self.silver, self.copper)

You can then consume like so:

>>> c = Currency(copper=653751735176)
>>> str(c)
'65,375,173 gold, 51 silver and 76 copper'
>>> c.copper
76
>>> c.silver
51
>>> c.gold
65375173
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. Now that you have created Currency and can compare both approaches and executions (including MarkM's thereof: What is your assessment of the relative merits? \$\endgroup\$ – greybeard Apr 25 at 3:14
2
\$\begingroup\$

Yet another possibility is to use a namedtuple to hold the coins.

from collections import namedtuple

class Currency(namedtuple("Currency", "gold silver copper")):

    def __new__(cls, copper):
        gold, copper = divmod(copper, 10000)
        silver, copper = divmod(copper, 100)
        return super().__new__(cls, gold, silver, copper)

    def __str__(self):
        return "{} gold, {} silver, {} copper".format(*self)

c = Currency(1234567)

print(c)

Here, I’ve hard-coded the conversion, but this could easily be adapted to variable exchange rate between gold, silver and copper coins, similar to other answers.

The benefit of the named tuple is the individual coin counts are accessible:

print(c.gold, c.silver, c.copper)

Since we now have a class for the Currency object, it would be possible to add basic arithmetic operators, so operations can be concisely expressed, if desired.

\$\endgroup\$

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.