Excel calculation engine

Question

We have currently rewritten an Excel calculation engine in F# and are looking to refactor the code to make it a lot more idiomatic and standardised.

One of the big problems with the code is that we have a massive function named companyModel that builds a CompanyModel type (a record type that holds some of the cached nested functions) from a Company type, IntercoTimeSeries type and DividendTrappedCashSolver type. The companyModel function consists of lots of small nested functions (nearly 2,000 lines of code!) that do calculations with the inputs and then produce a CompanyModel as a result.

One of the positives with the horrific implementation of companyModel is that all the nested functions have global access to the Company, IntercoTimeSeries and DividendTrappedCashSolver inputs so we don't need to pass these as arguments through all our individual nested functions. The issues with this are that the code is hard to test in isolation and is softly organised using #Region comments instead of say more rigidly using nested modules.

The first idea for refactoring was to replace the nested #Region comments with nested modules and to then put the nested functions in these nested modules. The companyModel function could then just call the last few functions in the last nested module to calculate a CompanyModel result.

However there are a few problems with this. These include:

1. We lose global access to Company, IntercoTimeSeries and DividendTrappedCashSolver inputs and these then have to be passed though all functions where required. Currently F# doesn't allow us to pass global parameters to modules (and nested modules) hence they must be past via the individual functions themselves and hence increasing the number of parameters and making each function more complex. We could maybe replace the modules with class types as these do let us pass global parameters, however unlike modules these can't be nested for organisation purposes.
2. To complicate matters there is heavy use of memoisation in the nested functions to enable caching of values in prior time periods and to reduce needless function recalculation in general. Having to pass these extra parameters to all the functions could then complicate the memoisation scheme which just doesn't feel right.

What is the best way to refactor the nested functions in the companyModel function to help with better testing in isolation but not change the logic too much with regards to making the functions more complex with more parameters and complicating the memoisation process?

I have added an extract of the companyModel function below along with the existing memoisation functions. If any more detail is required such as the types used by the functions let me know but I don't think these should impact a possible refactoring solution.

// Utility.fs
module Utility = 
 let inline memoise f = 
 let res = ref Unchecked.defaultof<_>
 let dict = System.Collections.Generic.Dictionary<int,_>(Time.timeLineLength) 
 fun v ->
 if dict.TryGetValue(v.Period, res) then
 !res
 else
 let res = f v 
 dict.Add(v.Period, res)
 res 
 let inline memoise2 f = 
 let res = ref Unchecked.defaultof<_>
 let dict = System.Collections.Generic.Dictionary<int,_>(Time.timeLineLength)
 fun u v ->
 if dict.TryGetValue(v.Period, res) then
 !res
 else
 let res = f u v 
 dict.Add(v.Period, res)
 res 
// CompanyModel.fs
module CompanyModel =
 let companyModel (c : Company, i : IntercoTimeSeries, s : DividendTrappedCashSolver) =
 // Alias Budget Model Database lookups
 let profitAndLoss = c.BudgetModelDatabase.ProfitAndLoss
 let balanceSheet = c.BudgetModelDatabase.BalanceSheet
 let cash = c.BudgetModelDatabase.Cash
 //#region - TimingFlags
 //#region -- DividendQuarter
 let dividendQuarter (t : Time) =
 let currMonth = Time.month t
 match c.DividendGroup with
 | DividendGroup.Tele -> 
 if currMonth = 3 then 4
 else if currMonth = 6 then 1
 else if currMonth = 9 then 2
 else if currMonth = 12 then 3
 else 0
 | _ -> 
 if currMonth = 2 then 4
 else if currMonth = 5 then 1
 else if currMonth = 8 then 2
 else if currMonth = 11 then 3
 else 0
 let dividendQuarterFlag t =
 dividendQuarter t <> 0
 //#endregion DividendQuarter
 //#region -- CompanyWindDown
 let companyWindDownPeriodStart =
 c.FinalMonth
 let companyWindDownFlag (t : Time) =
 t >= c.FinalMonth
 let cashflowTaxWindDownFlag (t : Time) =
 t.BudgetMonth >= c.FinalMonth.BudgetMonth - 8
 let profitAndLossTaxWindDownFlag (t : Time) =
 t.BudgetMonth >= c.FinalMonth.BudgetMonth - 12
 //#endregion CompanyWindDown
 //#region -- CompanySold
 let companySold t =
 match c.SoldMonth with
 | Some v -> t >= v
 | None -> false
 let companyNotSold t =
 match c.SoldMonth with
 | Some v -> t < v
 | None -> true
 //#endregion CompanySold
 //#endregion TimingFlags
 //#region - Opening Balances
 let openingBalanceCash = 
 memoise2 (fun closingBalanceCash (t : Time) ->
 if (t - 1).IsForecast then
 closingBalanceCash
 else
 balanceSheet.Cash (t - 1)
 )
 let openingBalanceIntercompanyDebt = 
 memoise2 (fun closingBalanceIntercompanyDebt (t : Time) ->
 if (t - 1).IsForecast then
 closingBalanceIntercompanyDebt
 else
 balanceSheet.IntercompanyLoan (t - 1) 
 ) 
 let openingBalanceRetainedEarnings = 
 memoise2 (fun closingBalanceRetainedEarnings (t : Time) ->
 if (t - 1).IsForecast then
 closingBalanceRetainedEarnings
 else
 - balanceSheet.RetainedEarnings (t - 1) 
 ) 
 let openingBalanceShareCapital = 
 memoise2 (fun closingBalanceShareCapital (t : Time) ->
 if (t - 1).IsForecast then
 closingBalanceShareCapital
 else
 - balanceSheet.ShareCapital (t - 1) 
 - balanceSheet.SharePremium (t - 1)
 ) 
 //#endregion
 //#region - Shared Functions
 let cashBalanceInterestRate t =
 Interest.libor t + Interest.cashBalanceInterestMargin
 let cashBalanceInterestReceivable =
 memoise2 (fun closingBalanceCash (t : Time) ->
 if not (companyWindDownFlag t) then 
 if t.IsForecast then
 openingBalanceCash closingBalanceCash t * cashBalanceInterestRate t / 12.0
 else
 0.0
 else
 0.0
 )
 let intercompanyInterestRate t =
 Interest.libor t + Interest.intercompanyInterestRate
 let intercompanyLoanInterest = 
 memoise2 (fun closingBalanceIntercompanyDebt (t : Time) ->
 if not (companyWindDownFlag (t - 1)) then
 (openingBalanceIntercompanyDebt closingBalanceIntercompanyDebt t) * intercompanyInterestRate t / 12.0
 else
 0.0
 )
 //#endregion
 //#region - Tax
 //#region -- Taxable Revenue Profit
 //#region --- Taxable Revenue Profit - Before (Budget model) Interest & Capital Allowances
 let taxableRevenueProfitExcludingInterestAndCapitalAllowances (t : Time) = 
 let inline taxableProfitCheck (t : Time) f flag =
 if flag then
 f t
 else
 0.0
 let adjustedBonus (t : Time) =
 - profitAndLoss.Bonus t + 
 if Time.isAfterModelEnd t then
 0.0
 else if Time.isEndOfFinancialYear t then
 Time.midRangeSumByFast (min (t + 1) Time.modelEnd) (min (t + 12) Time.modelEnd) cash.LtisPayments
 else
 0.0
 taxableProfitCheck t profitAndLoss.Ebt c.TaxableProfitFlags.IncludeEbtAfterIntercompanyRecharges +
 - taxableProfitCheck t profitAndLoss.AddBackDepreciationAndAmortisation c.TaxableProfitFlags.IncludeAddBackDepreciationAmortisation +
 - taxableProfitCheck t profitAndLoss.LessProfitOnDisposal c.TaxableProfitFlags.IncludeLessProfitOnDisposal +
 - taxableProfitCheck t profitAndLoss.LessSurrenderPremiums c.TaxableProfitFlags.IncludeLessSurrenderPremiums +
 - taxableProfitCheck t profitAndLoss.LessOtherNonAllowableItems c.TaxableProfitFlags.IncludeLessOtherNonAllowableItems +
 taxableProfitCheck t profitAndLoss.OnerousLeaseTaxableProfit c.TaxableProfitFlags.IncludePlusOnerousLeaseTaxableProfit +
 - taxableProfitCheck t profitAndLoss.OnerousLeaseAccountingProfit c.TaxableProfitFlags.IncludeLessOnerousLeaseAccountingProfit +
 taxableProfitCheck t profitAndLoss.IrrecoverableVatProfitAndLoss c.TaxableProfitFlags.IncludeAddIrrecoverableVat +
 taxableProfitCheck t adjustedBonus c.TaxableProfitFlags.IncludeReplaceProfitAndLossBonusWithNextYearsCashBonus
 //#endregion Taxable Revenue Profit - Before (Budget model) Interest & Capital Allowances
 //#region --- Interest
 let intercompanyInterestReceivableForTax =
 let intercompanyLoanOpeningBalance =
 Time.lastNonZeroBy balanceSheet.IntercompanyLoan
 memoise2 (fun closingBalances (t : Time) ->
 let interCompanyInterestOnOpeningBalance (t : Time) =
 match c.CompanyShortName with
 | CompanyShortName.TSP -> 0.0
 | _ -> if t.IsForecast then
 intercompanyLoanOpeningBalance * cashBalanceInterestRate t / 12.0 
 else 
 0.0
 if Time.isBeforeModelStart (t - 1) then
 0.0
 else if (t - 12).IsForecast then
 intercompanyLoanInterest (closingBalances.IntercompanyDebt) (t - 12)
 else 
 interCompanyInterestOnOpeningBalance (t - 1))
 let rec taxCashBalanceInterestReceivable =
 memoise2 (fun closingBalanceCash (t : Time) ->
 if Time.isAfterModelEnd (t + 1) then
 0.0
 else if t.IsForecast then
 cashBalanceInterestReceivable closingBalanceCash t 
 else 
 taxCashBalanceInterestReceivable closingBalanceCash (t + 1)
 )
 let rec currentYearInterestReceivable =
 memoise2 (fun closingBalanceCash (t : Time) ->
 if Time.isAfterModelEnd (t + 11) then
 0.0
 else if Time.isStartOfFinancialYear t then
 Time.midRangeSumByFast t (t + 11) (taxCashBalanceInterestReceivable closingBalanceCash)
 else
 currentYearInterestReceivable closingBalanceCash (t - 1) 
 )
 let rec assumedMonthlyInterest =
 memoise2 (fun closingBalanceCash (t : Time) ->
 if Time.isBeforeModelStart (t - 11) then
 0.0
 else if Time.isEndOfFinancialYear t then
 currentYearInterestReceivable closingBalanceCash t - assumedMonthlyInterest closingBalanceCash (t - 1) 
 else
 currentYearInterestReceivable closingBalanceCash (t - 12) / 2.0 / 12.0
 )
 let cashBalanceInterestReceivableForTax closingBalanceCash (t : Time) =
 assumedMonthlyInterest closingBalanceCash (t - 12)
 //#endregion Interest
 // 1,500 more lines of business logic code here all part of the companyModel function
 // ....

Answer 1

One of the positives with the horrific implementation of companyModel is that all the nested functions have global access to the Company, IntercoTimeSeries and DividendTrappedCashSolver inputs so we don't need to pass these as arguments through all our individual nested functions.

As the Mythbusters are fond of saying, "Well, there's your problem." This statement is directly contrary to the Dependency Inversion Principle -- depend on abstractions, not on concretions. As you mention in the very next sentence, you already realize this renders your code untestable. It is in no way a "positive".

Try refactoring your code to pass in the needed dependencies. This will have the effect of making your logic in that function stateless. That will enable you to write tests for your logic.

You'll have to attack one small module or function at a time. I'd start with the "innermost" layer of functionality, one with the smallest number of dependencies, and which produces a value that other functions depend upon. Then as you write tests for the modules that use this value, you're not trying to test that all the inner functions are correct, because you've already tested them.

It may sound like a big deal, but you said it's only 2000 lines. If it expands to 4000 lines or 8000 lines, so what? It's not like consuming a few extra kilobytes of storage are going to break the bank; and you're not dealing with decks of punched cards. A smaller program size is not an indicator of quality (unless you're programming the Mars Rover, an embedded module for an appliance, or some first person shooter game with a 72 frames per second refresh rate.) However, having unmaintainable, untestable code could yield disastrous results for your business. That's a lot more important than code size.

• jeremyh: Thanks for the feedback. I guess I was more asking how I should refactor the code rather than whether it should be refactored. Maybe I should alter the question to make that more specific.

Answer 2

While this does not directly address the issue of how to solve the larger problem with refactoring, reducing the amount and complexity of code should make it simpler to do that. Here are a couple of suggestions on how to do this (I don't have an F# compiler handy so please excuse any errors).

I would change

let dividendQuarter (t : Time) =
 let currMonth = Time.month t
 match c.DividendGroup with
 | DividendGroup.Tele -> 
 if currMonth = 3 then 4
 else if currMonth = 6 then 1
 else if currMonth = 9 then 2
 else if currMonth = 12 then 3
 else 0
 | _ -> 
 if currMonth = 2 then 4
 else if currMonth = 5 then 1
 else if currMonth = 8 then 2
 else if currMonth = 11 then 3
 else 0

to

let dividendQuarter (t : Time) =
 match c.DividendGroup with
 | DividendGroup.Tele ->
 match Time.month t with
 | 3 -> 4
 | 6 -> 1
 | 9 -> 2
 | 12 -> 3
 | _ -> 0
 | _ ->
 match Time.month t with
 | 2 -> 4
 | 5 -> 1
 | 8 -> 2
 | 11 -> 3
 | _ -> 0

or Update: Added (DividendGroup.Tele, _) -> 0 to match original implementation

let dividendQuarter (t : Time) =
 match (c.DividendGroup, Time.month t) with
 | (DividendGroup.Tele, 3) -> 4
 | (DividendGroup.Tele, 6) -> 1
 | (DividendGroup.Tele, 9) -> 2
 | (DividendGroup.Tele, 12) -> 3
 | (DividendGroup.Tele, _) -> 0
 | (_, 2) -> 4
 | (_, 5) -> 1
 | (_, 8) -> 2
 | (_, 11) -> 3
 | _ -> 0

according to taste.

---

You have a number of functions with the same pattern, for example:

 let openingBalanceShareCapital = 
 memoise2 (fun closingBalanceShareCapital (t : Time) ->
 if (t - 1).IsForecast then
 closingBalanceShareCapital
 else
 - balanceSheet.ShareCapital (t - 1) 
 - balanceSheet.SharePremium (t - 1)
 ) 

I would change those to something like this (maybe this idea can be applied in other areas as well):

let opening balance t =
 let transformed =
 memoise2 (fun closing (t : Time) ->
 if (t - 1).IsForecast then
 closing
 else
 balance t
 )
 transformed
let balanceCash t =
 balanceSheet.Cash (t - 1)
let alanceIntercompanyDebt t =
 balanceSheet.IntercompanyLoan (t - 1) 
let alanceRetainedEarnings t =
 - balanceSheet.RetainedEarnings (t - 1) 
let balanceShareCapital t =
 - balanceSheet.ShareCapital (t - 1) - balanceSheet.SharePremium (t - 1)
let openingBalanceCash = opening balanceCash t
let openingBalanceIntercompanyDebt = opening alanceIntercompanyDebt t
let openingBalanceRetainedEarnings = opening alanceRetainedEarnings t
let openingBalanceShareCapital = opening balanceShareCapital t

---

With regards to Company, IntercoTimeSeries and DividendTrappedCashSolver, you could put them in a single record, reducing the amount of parameters you have to pass around.

• jeremyh: Thanks for the reply! I think the first way you rewrote dividendQuarter using the nested match statements is a lot neater although might need to memoise Time.month t behind the scenes. We thought about wrapping the inputs into a single record, however were unsure what impact this might have on performance? Another option that we have investigated is passing the inputs as tuples. That way we can build up different combinations of inputs anonymously (as opposed to naming them using a record) for example not many functions use DividendTrappedCashSolver so this can be left out of most tuples.
• Morten Christiansen: @jeremyh I added another even more compact version which I had forgotten F# supports.
• jeremyh: Oh yeah that is quite neat and short, however do you think there would be some performance overhead in your code with respect to constructing the tuple of dividend group and month. The function dividendQuarter is called a lot!
• Morten Christiansen: @jeremyh Now that I think about it, I think I introduced a subtle bug in the last two versions. In your original code, and my first rewrite of dividendQuarter (DividendGroup.Tele, 6) would return 0, but in my code it would return 1 because it is matched by (_, 5). I'll see if I can fix it.
• Morten Christiansen: @jeremyh I don't know about the performance. I guess you will have to measure or at least use a disassembler to get an idea of the generated IL.