ChrisCScott / forecaster

A personal finances forecasting tool for Canadian retirement planning
Other
1 stars 2 forks source link

Limit dependence on Decimal and Money #56

Closed ChrisCScott closed 2 years ago

ChrisCScott commented 5 years ago

Various classes (among them Account and Person) cast their inputs to either Decimal or Money. None of the functionality of these base classes relies on the use of these types; it's assumed that the client code will want to use them (e.g. for localized display of currencies).

Rather than make this assumption, we should simply use whatever types are passed by client code. We can replicate the current functionality simply by passing Decimal and Money objects from client code.

This does raise two points of complexity:

  1. Comparisons: It is often necessary to determine whether a monetary value is less than, greater than, or equal to 0. We currently deal with this by subclassing Money to override comparison operators specifically when the operand is a non-Money 0-valued Number. It's a bit of a hack, and not one we'd want to push onto client code.
  2. Instantiation: We often return Money(0) or use it as a default value, without necessarily having first received an instance of a monetary value from client code. This makes it tricky to instantiate new monetary values with the appropriate type.

We can address these by storing a MONEY0 object - a zero-valued object of the appropriate type for monetary values. Consider whether this object should be global, class-specific, or instance-specific. I'm currently leaning toward instance-specific, perhaps by defining a property that returns a zero-valued object of the same type as some reference property. For example:

class Account(Ledger):
    def __init___ (self, balance, ...):
        self.balance = balance
        self._moneytype = type(balance)
        self._money0 = self._moneytype(0)
        ...

    @property
    def _MONEY0(self):
        if not isinstance(self.balance, self._moneytype):
            self._moneytype = type(balance)
            self._money0 = self._moneytype(0)
        return self._money0

You can imagine extending this. We could define a str-valued class or instance attribute that names the attribute to use for type-matching (e.g. self._moneyref="balance") and invoke getattr to acquire that attribute dynamically.

We could move this logic into a class and define some suitable methods (e.g. _setmoneytype could implement the two repeated lines above setting self._money* attributes) to make it reusable, although if it provides an __init__ method then we may need to consider whether init order and multiple inheritance make this a headache - do we need to call super().__init__ from Ledger? Do we need to assign self.balance = balance before calling super().__init__? (Would that break some Ledger code, given that balance is a recorded_property that requires some setup?) Another option is that it could lack an __init__ method and we could simply call _setmoneytype at the end of the inheriting object's __init__ method (or after setting the reference attribute and before calling any methods that need _MONEY0 to be defined). A further option would be to assign to the underlying _money* attributes in the class's __init__ using some default type (e.g. float) so that a sensible _MONEY0 object is available at all stages of __init__, though this may make bugs harder to track down.

ChrisCScott commented 3 years ago

On reflection, it's not clear that there's a good reason for using Decimal or Money at all in the base Ledger-derived classes. These could simply manipulate integer or floating-point values and leave it to client code to convert these to suitable Money types for display purposes. This would remove a considerable amount of complexity from the code - and it would probably help resolve issue #76 too.

ChrisCScott commented 2 years ago

Closed by #78.