Closed ChrisCScott closed 5 years ago
An alternative refactoring approach would be to break up Forecast
into multiple classes. Perhaps one class for each strategy class? Maybe also initial set-up and final processing classes? (Consider: ForecastIncome
, ForecastGrossContributions
, ForecastContributionReductions
, ForecastContributionTransactions
, ForecastReturns
, ForecastWithdrawals
, ForecastWithdrawalTransactions
, ForecastTax
, ForecastTotal
.)
This could be combined with the above-suggested recorded_property_cached
, and comes with the benefit of breaking up logic into separate, shorter classes with much fewer __init__
args.
The model revisions noted in #52 will help with this refactoring. It will allow most of detailed determination steps to occur in other classes and provides a structure that will likely make it easier to build in pensions and other benefits in the future. (E.g. perhaps pensions can be included in income at the start, and benefits can be included at the end, after withdrawals and taxes, so that tax treatment can be taken into account to adjust benefit rates - consider this more closely as part of the appropriate milestone.)
One item of note is that the per-account transactions steps might not be better-addressed by other classes. For example, leaving it to Forecast
to handle per-account transactions makes it hard to deal with non-retirement/non-debt accounts that are lumped in with lifecycle expenses. The key example is education accounts (e.g. RESPs); if Forecast
merely sees the lump sum amount of lifecycle expenses, how does it determine RESP contributions? It may be better to let the lifecycle-forecasting class handle that. Alternatively, we'd need to figure out how to communicate in a much more complicated way between the two classes (e.g. by passing time-series cashflow data back and forth).
Following a planning session, the following structure is proposed:
Forecast
has a number of SubForecast
attributes (e.g. IncomeForecast
, LivingExpensesForecast
), each of which manages its relevant Account
s (or other Ledger
objects), including recording transactions against them. These classes may be composed with Strategy
objects, and we might eventually merge the two.
Forecast
has two core responsibilities: (1) to track the pool of available money at the start of each SubForecast
invocation (e.g. recording inflows based on IncomeForecast
's projected employment income and recording outflows based on LivingExpensesForecast
's projected living expenses) and (2) to ensure that each SubForecast
is invoked in the appropriate order and with access to the pool of available money it has available to it. In a future version it may also provide statistics. Other functionality should be left to SubForecast
or lower-level classes.
Implementation Considerations
Each SubForecast
should have all previous SubForecast
's information available to it (or, at the very least, all previous SubForecast
s which a given SubForecast
requires, which could in theory be any of them.) Consider whether each SubForecast
should receive its dependencies as SubForecast.__init__
arguments, from Forecast
at invocation as **kwargs
, from Forecast
at Forecast.__init__
, or at some other time (setting attribute values post-__init__
?) I currently favour receiving them at Forecast.__init__
; consider whether we want to simply set (say) living_expenses_forecast.income_forecast = self.income_forecast
automatically, or whether we want to set it only if living_expenses_forecast
has declared an income_forecast
variable.
It's possible to pass information on available money as a scalar (Money
), but I usually think of inflows and outflows being managed by Forecast
as a defaultdict (of when: Money
pairs) called available
(or maybe transactions
, which is what similar dicts are called elsewhere in the code.) It's empty at the start of each year; each SubForecast
either mutates it on invocation or returns an updated copy. (SubForecast
s should probably retain a copy of available
as received.) Positive values reflect money available for use by SubForecast
s further down the invocation chain. For example, IncomeForecast
adds positive values (probably monthly), and LivingExpensesForecast
adds negative values. In general, withdrawals or distributions from accounts are positive (since they add cash to the pool) and contributions or payments are negative (since they remove cash).
Each invocation of SubForecast
is potentially computationally-expensive and so should probably be implemented as recorded_property_cached
. It's also not possible to know what the correct results are until the SubForecast
has received available
when invoked by Forecast
. Consider making available
a mandatory argument to next_year
, or otherwise ensuring that calculation of recorded_property_cached
is deferred until invocation.
To ensure correct ordering of SubForecast invocations, consider storing a list (which is ordered) of SubForecast
objects. They can then be called in order, with the available
result of each invocation being passed to the next. If we decide pass prior SubForecast
via Forecast
(e.g. as **kwargs
or at __init__
), consider also maintaining a list of keys so that we can programmatically pass SubForecasts
with the appropriate names. We'd probably want to add methods for appending and inserting SubForecast
s so that keys can be managed properly.
The methods currently in Forecast
for managing transactions can be moved to a SubForecast
base class. This provides most of the functionality we'll likely need already, but consider whether we need to extend that functionality to allow the `SubForecast to force the available pool of money to go negative at a particular point in time (e.g. to maintain minimum debt payments). Recall that, ordinarily, transactions get moved around to ensure that this doesn't happen.
Note that LivingExpensesForecast
could potentially be used by both ContributionForecast and WithdrawalForecast (both use a GrossTransactionStrategy
). Instead of amalgamating it, consider implementing a container class that calls one of two GrossTransactionStrategy
objects depending on whether or not the plannees are retired, thereby allowing the total amount needed for withdrawals to be determined simply by examining available
.
Currently,
Forecast
is written procedurally. The has the benefit of being clear to read and relatively easy to debug, but it's also somewhat brittle - changing the structure of the class involves carefully considering its impact on code earlier or later in the procedure.It could be refactored into a subclass of
Ledger
with each attribute written as arecorded_property_cached
. Then each attribute could simply provide a formula for determining its value based on other attributes; so long as there are no cyclic references, all attributes should be set automatically whennext_year
is called.It will likely be necessary to override
Forecast.next_year
to callnext_year
for eachAccount
andPerson
object before callingsuper().next_year
. (Consider whetherPerson
should be updated to callnext_year
for any of itsAccount
objects when itsnext_year
is called, similar to the logic inAccount
now. This would simplify the call forForecast
, which could ignoreAccount
s.)