Non-standardised data across multiple worksheets: Aggregation, Validation, Filtering

Every piece of business that gets written (E.G. an adviser's client puts an additional £10k payment into a pension plan) goes in a spreadsheet called the SubSheet. It's split into 8 worksheets for different kinds of business, each with lots and lots of columns. E.G. "Adviser" "First Name" "Investment Amount" "Commission Due" "Date Received" "File Check 1 Date" "File Check 1 by:" all that kind of stuff.

N.B. the worksheets do not have identical structures. So aggregation involves standardising all the tables into the same format as well as some other stuff.

Every month a report is put together manually, showing all the company's business, split out by adviser, type of business, provider (E.G. pension provider). This takes a lot of time and is very vulnerable to human error.

The project was to automate the process of creating this report. It works and is 100% accurate (so far as I can determine). It runs to about 2,000 lines across 3 Modules.

What I'm after with this question:

Using this project as an example of my current level of proficiency with best practices, I'm after specific suggestions on how it can be / could have been, structured / written to improve the following criteria:

-
Readability: Ability for somebody who is not me to come in blind, and (relatively) easily figure out how the whole thing works and fix some problem that's cropped up.

-
Robustness: Designing subs/functions to deal with variable cases. (E.G. I used to do a lot of using exact cell references, so if you added a column, the whole macro broke, in multiple places. Nowadays, I know better.)

-
Reusability: Designing subs/functions/the entire project so they can be easily re-purposed for future projects. (E.G. I rebuilt the sub for getting worksheet data into an array so it could be literally copy-pasted into any future project)

-
Scalability: Things like version control, Splitting subs between Modules, Controlling variable Scope, Adding more control layers, commenting, etc. Basica

A wise programmer once told me


What a fascinating solution! It seems that for every smart decision you made, you also threw in a poor decision or two.

I now completely understand where he was coming from, because I feel the same way about this code here. It's a beautiful mix of good and bad decisions.

Let's look at Module 1.

Option Explicit
Option Compare Text '/ Case Insensitive

Off to a good start. You're using Option Explicit. I like that. I'm honestly not sure why the language design team ever made it an option at all.

Sub Generate_Adviser_Submissions_Report()

It's a good descriptive name, but it's got a few problems. First, it's implicitly public. We should be as explicit as possible about scope. Please don't make me remember what the default is, tell me. Secondly, I really don't like your underscore naming convention. You see, the underscore has a special place in VBA/VB6. It indicates one of two things.

-
An event procedure.

Button1_OnClick()

-
An interface implementation.

IEngine_Start()

Using underscores in regular methods confuses things and makes it hard impossible to pick these elements out just by glancing at the code.

Application.ScreenUpdating = False
Application.EnableEvents = False
Application.Calculation = xlCalculationManual

That'll speed things up! Cool! But what happens if you get a runtime error? You're user is left with calculation set to manual and hopelessly confused as to why their formulas aren't updating. You really need an error handler here to make sure this stuff always get set back to the way it was. It's also best to capture the value of Application.Calculation to use for setting it back, because the user might have already had it set to manual, so again following the principle of least surprise, you wouldn't want to set it to automatic in the error handler, you'll want to restore it to whatever the user had it set to.

'/======================================================================================================================================================
'/  Author:  Zak Armstrong
'/  Email:   zak.armstrong@company.co.uk
'/  Date:    12/August/2015
'/  Version: 0.2

Cool! Now I know who to contact if I have a question about the code when I'm maintaining it.

'/  Is Called By:   None
'/
'/  Calls:          Open_Workbooks
'/                  Initialise_Worksheets
'/                  Initialise_Collections_And_MetricHeadings
'/                  Initialise_Providers_And_Advisers
'/                  Insert_arrAscentric_LifeCo_Column

Not so cool. These kinds of comments tend to lie because they never get maintained. If they do get maintained, they're just sucking up valuable development time. If you need to know what method is calling another, use an add-in like MZ-Tools or Rubberduck. There is a caveat to this. Those tools can't detect when a function is called as a UDF, from an Access Macro, or from an external VBA project in another workbook. In that case, I completely recommend you keep the "Is Called By" section, but repurpose it to documenting calls that can't be detected by software.

'/  Description:    All Company Wealth Business is contained in the Subsheet. This macro produces adviser totals for business (assets and fees) in the previous year
'/                  (month by month breakdown) by aggregating the subsheet into one giant table and then assigning each piece of business to an adviser, a Month and a business type.
'/                  The report can then be easily configured for any desired outputs (E.G. by adviser, by provider, by type of business)

This is utterly fantastic. I wish more people (including myself) left these kind of purpose documenting comments.

'/  Change Log:     | Author            | Date          | Description of Changes

I get it. I do. It's hard to source control VBA code, but there are options out there (I created a few myself) and this change log doesn't really do much for you. You couldn't get the code back anyway. If you're going to keep a change log, use an external Excel workbook for the task. I'm just speaking from years of professional VBA development in a corporate environment. Life was just easier when we moved the change log out of the code. It removes the distraction of all the comments and lets you focus on the code.

```
Dim arrNewClient() As Variant '/ An array to hold all data on the "New Client Investment" Sheet
Dim arrExistingClient() As Variant '/ An array to hold all data on the "Existing Client Investment" Sheet
Dim arrGroupSchemes() As Variant '/ An array to hold all data on the "Group Schemes" Sheet
Dim arrOther() As Variant '/ An array to hold all data on the "Other" Sheet
Dim arrMcOngoing() As Variant '/ An array to hold all data on the "MC Ongoing" Sheet
Dim arrJhOngoing()

StackExchange Code Review Q#101369, answer score: 8