snippetMinor
Aggregating data from multiple worksheets into a consistent format
Viewed 0 times
formatconsistentintoaggregatingmultipleworksheetsfromdata
Problem
First off, this is the 3rd (and probably last) review of the project in question. You can find the previous question here.
The Macro accesses a workbook containing 8 worksheets each with similarly structured but not identical tables of data (submitted business for my company). It then filters this data for desired columns and aggregates it into a separate workbook.
As before, I'd like advice/feedback on improving the following:
-
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 and/or to reliably fail when given unintended arguments.
-
Reusability: Designing subs/functions/the entire project so they can be easily re-purposed for future projects.
What Changed: The order in which the Macro does things is roughly the same, there's been a significant amount of further refactoring, renaming subs/functions/variables and general incorporating of previous advice.
Since this is (hopefully) the end of this particular project, I would also appreciate feedback on what I'm doing right.
Module 1: M1_Public_Variables_Constants
N.B. I'm aware that making all the declarations line up like this isn't an efficient use of time, but since it's already been done, I'm not about to go and spend more time deliberately undoing it.
```
Option Explicit
Option Compare Text
'/ Workbooks
Public WbSubsheet As Workbook '/ Contains all Lumin Wealth submitted Business
Public WbAdviserReport As Workbook '/ Will Contain an aggregation of the subsheet and a submission report (by month) for each adviser
'/ Adviser Report worksheets
Public WsAggregatedData As Worksheet '/ Will contain the aggregated subsheet data
Public WsAdviserReport As Worksheet '/ Will contain the submis
The Macro accesses a workbook containing 8 worksheets each with similarly structured but not identical tables of data (submitted business for my company). It then filters this data for desired columns and aggregates it into a separate workbook.
As before, I'd like advice/feedback on improving the following:
-
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 and/or to reliably fail when given unintended arguments.
-
Reusability: Designing subs/functions/the entire project so they can be easily re-purposed for future projects.
What Changed: The order in which the Macro does things is roughly the same, there's been a significant amount of further refactoring, renaming subs/functions/variables and general incorporating of previous advice.
Since this is (hopefully) the end of this particular project, I would also appreciate feedback on what I'm doing right.
Module 1: M1_Public_Variables_Constants
N.B. I'm aware that making all the declarations line up like this isn't an efficient use of time, but since it's already been done, I'm not about to go and spend more time deliberately undoing it.
```
Option Explicit
Option Compare Text
'/ Workbooks
Public WbSubsheet As Workbook '/ Contains all Lumin Wealth submitted Business
Public WbAdviserReport As Workbook '/ Will Contain an aggregation of the subsheet and a submission report (by month) for each adviser
'/ Adviser Report worksheets
Public WsAggregatedData As Worksheet '/ Will contain the aggregated subsheet data
Public WsAdviserReport As Worksheet '/ Will contain the submis
Solution
I'm aware that making all the declarations line up like this isn't an efficient use of time, but since it's already been done, I'm not about to go and spend more time deliberately undoing it.
Thanks, you just made a feature request for Rubberduck 2.0!
You have object references - why do you use
Instead of an implicit reference to the active worksheet (with the
I like this, but I don't get why you need the line continuation here, nor why you're forcing
should be this:
Why does
should be:
Again, the off-standard indentation is a bit off-putting, and I'm not sure what to think of the vertical whitespace:
I'd have formatted it like this:
Notice the variable is declared closer to its usage, and that all executable instructions are at the same level of indentation - but I don't completely disagree with treating
Thanks, you just made a feature request for Rubberduck 2.0!
You have object references - why do you use
Activate?WbAdviserReport.Activate
WsAggregatedData.Activate
...
Set rngHolder = Cells(i, LB2)Instead of an implicit reference to the active worksheet (with the
Cells call), use an explicit reference, and get rid of the Activate calls:Set rngHolder = WsAggregatedData.Cells(i, LB2)I like this, but I don't get why you need the line continuation here, nor why you're forcing
strErrorMessage to be passed ByVal, since ErrorMessage already states that the strErrorMessage parameter is passed by value:If bError _
Then
strErrorMessage = "Unidentified Adviser - Row: " & i & "Text: " & rngHolder.Text
ErrorMessage (strErrorMessage)
End Ifshould be this:
If bError Then
strErrorMessage = "Unidentified Adviser - Row: " & i & "Text: " & rngHolder.Text
ErrorMessage strErrorMessage
End IfWhy does
MissingDataHeadingsHandler spell the condition differently?If bErrorFound = True Then ErrorMessage (strErrorMessage)should be:
If bErrorFound Then ErrorMessage strErrorMessageAgain, the off-standard indentation is a bit off-putting, and I'm not sure what to think of the vertical whitespace:
Public Function WorkbookIsOpen(ByVal strTargetName As String) As Boolean
Dim wbTest As Workbook
On Error Resume Next
Set wbTest = Workbooks(strTargetName)
WorkbookIsOpen = (wbTest.Name = strTargetName)
On Error GoTo 0
End FunctionI'd have formatted it like this:
Public Function WorkbookIsOpen(ByVal strTargetName As String) As Boolean
On Error Resume Next
Dim wbTest As Workbook
Set wbTest = Workbooks(strTargetName)
WorkbookIsOpen = (wbTest.Name = strTargetName)
On Error GoTo 0
End FunctionNotice the variable is declared closer to its usage, and that all executable instructions are at the same level of indentation - but I don't completely disagree with treating
On Error Resume Next...On Error GoTo 0 as a code block.Code Snippets
WbAdviserReport.Activate
WsAggregatedData.Activate
...
Set rngHolder = Cells(i, LB2)Set rngHolder = WsAggregatedData.Cells(i, LB2)If bError _
Then
strErrorMessage = "Unidentified Adviser - Row: " & i & "Text: " & rngHolder.Text
ErrorMessage (strErrorMessage)
End IfIf bError Then
strErrorMessage = "Unidentified Adviser - Row: " & i & "Text: " & rngHolder.Text
ErrorMessage strErrorMessage
End IfIf bErrorFound = True Then ErrorMessage (strErrorMessage)Context
StackExchange Code Review Q#102302, answer score: 5
Revisions (0)
No revisions yet.