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.

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.