HiveBrain v1.2.0
Get Started
← Back to all entries
snippetMinor

Aggregating data from multiple worksheets into a consistent format

Submitted by: @import:stackexchange-codereview··
0
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

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 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 If


should be this:

If bError Then
    strErrorMessage = "Unidentified Adviser - Row: " & i & "Text: " & rngHolder.Text
    ErrorMessage strErrorMessage
End If


Why does MissingDataHeadingsHandler spell the condition differently?

If bErrorFound = True Then ErrorMessage (strErrorMessage)


should be:

If bErrorFound Then ErrorMessage strErrorMessage


Again, 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 Function


I'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 Function


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 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 If
If bError Then
    strErrorMessage = "Unidentified Adviser - Row: " & i & "Text: " & rngHolder.Text
    ErrorMessage strErrorMessage
End If
If bErrorFound = True Then ErrorMessage (strErrorMessage)

Context

StackExchange Code Review Q#102302, answer score: 5

Revisions (0)

No revisions yet.