Building a set of flags based on data from a source workbook

This takes in an array of data built from a source workbook, builds a set of "flags" based on the data in each row of the array. Then it creates a new finalRng array that will, based on a set of logic from the business, create new rows based on the data.

I think there are a ton of ways to accomplish what I am looking for but this code is what I have come up with so far (code is in a UserForm module):

```
Option Explicit
Dim nonLoanCodes As Variant
Dim sourceColumns As Variant
Dim finalRng()
Dim trimmedRange As Variant
Dim log As New Logger
Dim beforeTaxPercentSum As Double
Dim beforeTaxFlatSum As Long
Dim rothPercentSum As Double
Dim rothFlatSum As Long
Dim logMessage As String
Dim strFirstFile, strSecondFile, strThirdFile As String
Dim wbkFirstFile, wbkSecondFile, wbkThirdFile, wbkConfigFile As Workbook

Private Sub btnBuildImportFile_Click()
'
' This function will build a 401k/Loan Worksheet for upload into UltiPro
'

On Error GoTo ErrorHappened
Dim lastRow As Long
Dim allRowFlags() As FlagBag
Dim payrollDate As String
Dim cell As Range

Application.DisplayAlerts = False
payrollDate = cmbPayrollDate.Value

'Declare source and destination workbooks
strFirstFile = lblFileName.Caption
strSecondFile = ThisWorkbook.path & "\template.xlsx"
strThirdFile = ThisWorkbook.path & "\ultiImport_" + Format(Now, _
"yyyy_mm_dd-hh_mm") + ".xlsx"
Set wbkFirstFile = Workbooks.Open(strFirstFile)
Set wbkSecondFile = Workbooks.Open(strSecondFile)

'Function call to validate whether the chosen source file is formatted correctly
If IsValidImportSheet(wbkFirstFile) = False Then
lblFileName.Caption = ""
Label2.Caption = ""
wbkFirstFile.Close
Else
'Function call to get last row from source sheet and build the source range and destination columns
lastRow = GetLastRowOnSheet(wbkFirstFile)

'Grab only the necessary values from the source worksheet and put them into an array

It's not clear what FlagBag might be. If it's a class, then the variable rowFlag is used but never assigned, which could be a bug. If it's a user-defined type, ...consider making it a class, since you're passing it around as parameters.

I promised I'd run Rubberduck 2.0 code inspections on your code, so here it goes:

]

After ignoring a few false positives, about 60 issues remained.

Code quality issues

-
Implicit ByRef parameter: parameters in VBA are passed ByRef by default. In many languages (including VB.NET), parameters are passed ByVal by default, which can be confusing; consider being explicit about how you're passing VBA parameters. This applies to lngCol in the Col_Letter function, and to the fileName parameter of the GetDirectory function.

-
Return type is implicitly 'Variant': functions return a value, and it's very seldom that this value needs to be an actual Variant - and when it does, it's still best to be explicit about the function's return type (As Variant). This applies to functions GetDirectory (which should be returning a String) and CalculateRowValues (which... see next point).

-
Non-returning function or property getter: this is either a confusing API, or a bug: functions should return a value - if the return value isn't assigned, the function isn't returning anything. This applies to the CalculateRowValues function, which looks like it should be a Sub, not a Function.

-
Parameter can be passed by value: a parameter that's passed ByRef but that isn't assigned a new value, has very little reasons to not be passed ByVal. This is purely semantics, but it can avoid introducing bugs when maintaining/refactoring the code.

-
Parameter is not referred to: nevermind that one, it pointed to the Logger.LogEntry stub method that I introduced to get the code to compile.

-
Variable is not referred to: variable payrollDate is never used in the Click handler for btnBuildImportFile. It's never referred to, but you're still assigning to it - the assignment is basically a no-op, and the variable could safely be removed.

-
Unassigned variable: the rowFlag variable in CalculateRowValues is never assigned. If FlagBag is a class (like I made it here... that's why I'm getting this inspection result), you have a runtime error 91 there. If it's a UDT, all is good.

-
Variable is used but not assigned: the allRowFlags() array is passed to the CalculateRowValues function/procedure from the BtnBuildImportFile_Click handler, but it's not initialized or used afterwards - perhaps it should be local to CalculateRowValues?

Language opportunities

-
Empty string literal: consider preferring vbNullString over "". It better communicates the intent of the code, and being a null string pointer it doesn't use any memory (vs. "" which takes up... 2 whole entire bytes).

-
Variable is implicitly 'Variant': a variable whose type isn't specified is implicitly declared as Variant, which isn't typically what you intend to do. For example, only the last variable is of the specified type here, strFirstFile, strSecondFile, wbkFirstFile and wbkSecondFile are all Variant:

Dim strFirstFile, strSecondFile, strThirdFile As String
Dim wbkFirstFile, wbkSecondFile, wbkThirdFile, wbkConfigFile As Workbook

this is a common beginner mistake, which leads to...

Maintainability & readability issues

-
Instruction contains multiple declarations: avoid declaring more than 1 variable on a single instruction/line. Your code has 5 instances of this issue, and every time, you're declaring implicit Variant variables without realizing.

-
Implicitly public member: module members in VBA are Public by default. Consider specifying an explicit access modifier, and making members only as visible as they need to be.

-
Use meaningful names: avoid 1-3 character identifiers and disemvoweling, and prefer names that can be pronounced. What does pth mean? Also, consider renaming Label2 and Label3 (and avoid numeric suffixes altogether) so that it's clear what their purpose is. There are exceptions, of course: i is commonly used with a For loop; ws and wb are commonly used for Worksheet and Workbook objects.

+
Other things that Rubberduck didn't pick up:

If rowFlag.fiftyPlus = True Then

Can be written as:

If rowFlag.fiftyPlus Then

Similarly:

And IsEmpty(trimmedRange(i, 2)) = False Then

Can be written as:

And Not IsEmpty(trimmedRange(i, 2)) Then

Avoid comparing Boolean values to Boolean literals in Boolean expressions, it's... redundant ;-)

Looking at the individual code blocks, it looks very much like a lot of copy/paste is going on:

ReDim Preserve finalRng(5, cnt)
        finalRng(0, cnt) = trimmedRange(i, 1)
        finalRng(2, cnt) = trimmedRange(i, 5)
        finalRng(5, cnt) = nonLoanCodes(8, 1)
        rothFlatSum = rothFlatSum + trimmedRange(i, 5)
        cnt = cnt + 1

This bod

StackExchange Code Review Q#116661, answer score: 3