alxwrd / coinhandler Goto Github PK

Coins can be created by subclassing Coin, but once created they can't be removed.

See: https://stackoverflow.com/q/52428679/7220776

Currently, the only valid coin set is GBP. It would be good to be able to use different currencies.

The file structure could become

coinhandler/
├── __init__.py
├── coinhandler.py
├── collections.py
└── coins/
    ├── __init__.py
    ├── gbp.py
    ├── usd.py
    └── etc..

With usage being

import coinhandler
from coinhandler.coins import gbp  # Required, if not all coins will be `Coin`

__init__.py for coins/ will contain the base definition of Coin.

This is related to #2, with the added thought being it'd be nice to be able to remove a whole module.

coinhandler.Coin.remove_subcoins(gbp)

This library seems like a cool idea, looking good!

I jumped into the code and noticed there's a file called collections.py, but 'collections' is already a thing in Python:

This might cause namespacey problems, and could slightly hinder readability - I saw collections was being imported in coinhandler.py and briefly thought it was referring to Python's own collections.

Might be worth renaming this to something more specific, although it's not really a problem if you want to keep the name the same

From the README:

When using the Coin factory class, a valid coin value should be used. Not doing so can create undesirable Coin objects.

 >>> coin = Coin(23)
OnePence(23)

# Use a CoinCollection instead!

>>> CoinCollection.from_value(23)
CoinCollection(TwentyPence(1), TwoPence(1), OnePence(1))

Re-thinking this I think it should be invalid to enter a state where an undesirable coin is about to be created (OnePence(23)).

I feel the correct way to handle this is to either raise an InvalidCoin exception. If a Coin has an .amount greater than 1, it's invalid.