Creating a pseudo Pivot Table / Database using a 4-D array

Why am I not just using a Pivot Table / Database?

• a) I've never ever used either before. And I don't have time to learn how before this project needs to actually be finished.

• b) The final output is going to be a report for the Company Directors. As such, the flexibility of choosing which bits of data to output in what format in which table on what sheet is highly useful.

I already have my raw data aggregated, validated and consistently formatted in a single table. This part of the Macro takes each line and:

• Determines which Financial Adviser is involved.

• Determines what type of business is involved (investments, insurance, premiums, invoives, management fees etc.).

• Determines which Financial Service provider is involved, if any (e.g. insurance providers, fund managers etc.).

• Determines which month the business was transacted in.

then aggregates the results into a 4-D array (Advisers, BusinessType, Providers, Month).

I am aware that alongside a pivot table, learning how to use dictionaries and an actual database would be useful. Given that those weren't options here, is there any other feedback you can offer?

N.B. I've tried to make my macro as robust as possible, so there's a lot of Find/Check position of in . This report is going to primarily be used by other people, and absolutely has to be 100% accurate, so I feel it's warranted.

Also, some of the basic methods aren't included here. You can safely assume that they do what they say they do.

Module 10: M10_Allocate_Business

Public Sub AllocateBusinessToAdvisersProvidersMonthsAndMetrics()

        PutSheetDataInArray WbAdviserReport, WsAggregatedData, ArrAggregatedData

        FindAllAdvisers

        FindAllProviders

    ReDim ArrAllocatedBusiness(0 To UBound(ArrAdvisers), 0 To ColMetrics.Count, 0 To UBound(ArrProviders), 0 To 13)

        PrepareAllocatedBusinessHeadings

        AllocateAggregatedBusiness

End Sub

```
Public Sub FindAllAdvisers()

Dim arrHeadingsRow A

Code is clean, the Hungarian naming is just slightly annoying, but variables have meaningful names, and that's good.

If I had to bring up just one thing, I'd bring up the non-standard indentation. I see your patterns there, and you seem to be following some convention - but it's non-standard nonetheless: Dim and ReDim are executable statements just like any other, I'm not sure why you're doing this:

FindAllProviders

ReDim ArrAllocatedBusiness(0 To UBound(ArrAdvisers), 0 To ColMetrics.Count, 0 To UBound(ArrProviders), 0 To 13)

    PrepareAllocatedBusinessHeadings

Not a big thing, it's just... distracting.

Also...

For i = LB1 + 1 To UB1
    For j = LB2 + 1 To UB2
    For k = LB3 + 1 To UB3
       '...
    Next k
    Next j
    Next i

I see what you're doing, but I don't agree. Nested loops should look like nested loops, and them being indented as such makes it much easier to see:

For i = LB1 + 1 To UB1
        For j = LB2 + 1 To UB2
            For k = LB3 + 1 To UB3
               '...
            Next k
        Next j
    Next i

Stackin/aligning them the way you did, makes it look like you somewhat want to hide the fact that you've got a 3-layer nested loop going on.

Case Is = ColMetrics.Item("Recurring")
            '/ do nothing

Why bother then? And if you're only ever going to run code when strTypeOfBusiness is not "Recurring", then the Select Case block reeks of YAGNI here, a regular If block would raise fewer questions eyebrows, especially with a comment that explains why we only care about non-recurring business here, when other methods handle several types differently.

Looking deeper, I'm not sure I like CheckErrorConditionsBusinessType at all. If should be returning that Boolean value (with an Optional ByRef outMessage = vbNullString), and leave it up to the caller to determine what to do with the return value, and whether to pop a messagebox or stick the message in the host app's statusbar.

The conditions should all be mutually exclusive, or each block should include an early return (Exit Function) - otherwise you make it look like the last block is the most important one, because it will always execute and overwrite whatever validation errors occurred before that, effectively having executed as many lines of code and made as many assignments for no reason: just return and report the first error, and don't bother validating the other conditions - the validation has already failed anyway.

StackExchange Code Review Q#102515, answer score: 3