8
\$\begingroup\$

Couple of items to make note of first:

  • Some of the code is inherited the other has been refactored by me. The procedures that have an underscore in them I am still working on refactoring.
  • I am still working through naming cells that I feel need to be named.
  • This code below was re-factored once with help from @TinMan on This Post

All of the code below works and I dont have any real performance issues with it, but the issue I am running into is structuring the procedures to be easier to maintain and I fear that I am actually making the code harder to maintain.

The long and short of this code is it looks to see what forms to create and the data to populate those forms with based on certain information being entered that are in the first 9 rows of Sheet1(Data Entry) and a few other cells. The code for populating the actual forms is not my concern; only the organization of the code and the use of the procedures I have created.

Class Module

'WireCustInfo Class
Option Explicit
Public cust_Name As String
Public cust_Address As String
Public cust_CityStateZip As String
Public cust_Zip As String
Public cust_HomePhone As String
Public cust_CellPhone As String
Public cust_Phone As String
Public cust_BSA As String
Public TableName As String
Public ErrNumber As Long
Public Property Get CustomerName() As String
    CustomerName = cust_Name
End Property
Public Property Let CustomerName(value As String)
    CustomerName = value
End Property
Public Property Get CustomerAddress() As String
    CustomerAddress = cust_Address
End Property
Public Property Let CustomerAddres(value As String)
    CustomerAddress = value
End Property
Public Property Get CustomerCityStateZip() As String
    CustomerCityStateZip = cust_CityStateZip
End Property
Public Property Let CustomerCityStateZip(value As String)
    CustomerCityStateZip = value
End Property
Public Property Get CustomerZip() As String
    CustomerZip = cust_Zip
End Property
Public Property Let CustomerZip(value As String)
    CustomerZip = value
End Property
Public Property Get CustomerHomePhone() As String
    CustomerHomePhone = cust_HomePhone
End Property
Public Property Let CustomerHomePhone(value As String)
    CustomerHomePhone = value
End Property
Public Property Get CustomerCellPhone() As String
    CustomerCellPhone = cust_CellPhone
End Property
Public Property Let CustomerCellPhone(value As String)
    CustomerCellPhone = value
End Property
Public Property Get CustomerBSA() As String
    CustomerBSA = cust_BSA
End Property
Public Property Let CustomerBSA(value As String)
    CustomerBSA = value
End Property

Constant Variables (saved in their own standard module)

Public Const CONNECTIONSTRING As String = Redacted for public viewing
Public Const BRANSON As String = "bhschlp8.jhadat842.cfmast cfmast"
Public Const CHARLOTTE As String = "cncttp08.jhadat842.cfmast cfmast"
Public Const CONNECTIONERROR As Long = -2147467259
Public Const RECURRINGOUTGOINGIDMETHOD = "The customer must have a Wire Transfer Agreement on file." & _
                                         "The Customer must be physically present to establish a Recurring Request."
Public Const OUTGOINGIDMETHOD = "If the customer has a Wire Transfer Agreement on file, " & _
                                "then we must use the Code Word/Pass Phrase/PIN listed in the agreement (unless the request was made in person)."
                                
Public Const RECURRINGWORKBOOK As String = "L:\Operations\Wire Transfers\Recurring Requests.xlsx"

Public DATAENTRY As Worksheet

Sheet1 Object

Option Explicit
Private Sub RecuringList_Click()
    Workbooks.Open RECURRINGWORKBOOK
End Sub

Private Sub Worksheet_Change(ByVal Target As Range)

    Application.ScreenUpdating = False
    Application.EnableEvents = False

'*******************************************************
'For ease of code maintenance the following procedures *
'that start with "Entry" and CreateAgreementRecurring  *
'are in the ChangeFormatEvents Module.                 *
'*******************************************************
    
    Select Case Target.Address
        Case Is = "$B$4"  'deIncWire
            EntryB4
        Case Is = "$B$5"  'deOutgoingWireDDALoan
            EntryB5
        Case Is = "$B$6"  'deOutgoingLoan
            EntryB6
        Case Is = "$B$7"  'deOutgoingCM
            EntryB7
        Case Is = "$B$8"  'deOutgoingBrokered
            EntryB8
        Case Is = "$B$9"  'deOutgoingInternal
            EntryB9
        Case Is = "$B$10", "$B$11"  'deNewTransferAgreement, deNewRecurringRequest
            Hide_All
            With DATAENTRY
                If Not .Range("deNewTransferAgreement").value = vbNullString Then
                    CreateAgreementRecurring CreateNewAgreement:=True, CreateRecurringRequest:=False
                End If
                
                If Not .Range("deNewRecurringRequest").value = vbNullString Then
                    CreateAgreementRecurring CreateNewAgreement:=False, CreateRecurringRequest:=True
                End If
            
                If Not .Range("deNewTransferAgreement").value = vbNullString And Not .Range("deNewRecurringRequest").value = vbNullString Then
                    CreateAgreementRecurring CreateNewAgreement:=True, CreateRecurringRequest:=True
                End If
            End With
            
        Dim wireTypeIs As String, CIFNum As String
        
        Case Is = "$B$103"
            If DATAENTRY.Range("B103") <> vbNullString Then
                CIFNum = DATAENTRY.Range("B103").value
                wireTypeIs = "Incoming"
                CIFGrab CIFNumber:=CIFNum, WireType:=wireTypeIs
            Else
                DATAENTRY.Range("B104:B107") = vbNullString
            End If
        Case Is = "$B$205"
            EntryB205
        Case Is = "$B$206"
            If DATAENTRY.Range("B206").value <> vbNullString Then
                CIFNum = DATAENTRY.Range("B206").value
                wireTypeIs = "OutGoingDDALoan"
                CIFGrab CIFNumber:=CIFNum, WireType:=wireTypeIs
            Else
                DATAENTRY.Range("B207:B211") = vbNullString
            End If
        Case Is = "$B$227"
            EntryB227
        Case Is = "$B$269"
            EntryB269
        Case Is = "$B$306"
            EntryB306
        Case Is = "$B$307"
            If DATAENTRY.Range("B307") <> vbNullString Then
                CIFNum = DATAENTRY.Range("B307").value
                wireTypeIs = "OutGoingLoan"
                CIFGrab CIFNumber:=CIFNum, WireType:=wireTypeIs
            Else
                DATAENTRY.Range("B308:B312") = vbNullString
            End If
        Case Is = "$B$331"
            EntryB331
        Case Is = "$B$373"
            EntryB373
        Case Is = "$B$406"
            EntryB406
        Case Is = "$B$407"
            If DATAENTRY.Range("B407") <> vbNullString Then
                CIFNum = DATAENTRY.Range("B407").value
                wireTypeIs = "OutGoingCM"
                CIFGrab CIFNumber:=CIFNum, WireType:=wireTypeIs
            Else
                DATAENTRY.Range("B408:B411") = vbNullString
            End If
        Case Is = "$B$425"
            EntryB425
        Case Is = "$B$506"
            If DATAENTRY.Range("B507") <> vbNullString Then
                CIFNum = DATAENTRY.Range("B507").value
                wireTypeIs = "OutGoingBrokered"
                CIFGrab CIFNumber:=CIFNum, WireType:=wireTypeIs
            Else
                DATAENTRY.Range("B507:B510") = vbNullString
            End If
        Case Is = "$B$610"
            EntryB610
        Case Is = "$B$5004"
            EntryB5004
        Case Is = "$B$5105"
            If DATAENTRY.Range("B5105") <> vbNullString Then
                CIFNum = DATAENTRY.Range("B5105").value
                wireTypeIs = "Recurring"
                CIFGrab CIFNumber:=CIFNum, WireType:=wireTypeIs
            Else
                DATAENTRY.Range("B5106:B5110") = vbNullString
            End If
        Case Is = "$B$5118"
            EntryB5118
    End Select

    Application.ScreenUpdating = True
    Application.EnableEvents = True


End Sub

Procedures (Stored in Different Standard Modules)

Option Explicit
Dim Unique_Identifier As String
Dim Wire_Type As String
Public Sub EntryB4()
    With DATAENTRY
        Hide_All
        Select Case .Range("B4")
            Case Is <> ""
                .Range("A100:A199").EntireRow.Hidden = False
                .Range("B101").Select
                .Range("B5") = ""
            Case Else
                .Range("B5").Select
        End Select
    End With
    Sheet5.Visible = xlSheetVisible 'Confirmation-Incoming
End Sub
Public Sub EntryB5()
    With DATAENTRY
        Hide_All
        If Not .Range("B5") = vbNullString Then
            Select Case IsNumeric(.Range("B5"))
                Case Is = False
                    .Range("A200:A211").EntireRow.Hidden = False
                    .Range("A216:A227").EntireRow.Hidden = False
                    .Range("C220") = OUTGOINGIDMETHOD
                    .Range("B201").Select
                    With ThisWorkbook
                        Sheet7.Visible = xlSheetVisible 'Checklist
                        Sheet4.Visible = xlSheetVisible 'Confirmation-Outgoing-1
                        Sheet2.Visible = xlSheetVisible 'Wire Transfer Request-1
                    End With
                Case Is = True
                    CIFGrab CIFNumber:=.Range("B206").value, WireType:="OutGoingDDALoan"
                    .Range("A200:A220").EntireRow.Hidden = False
                    .Range("A222,A226:A282").EntireRow.Hidden = False
                    .Range("C220") = RECURRINGOUTGOINGIDMETHOD
                    Unique_Identifier = .Range("B5").value
                    Wire_Type = "Deposit/Loan"
                    Call Find_Recurring(Unique_Identifier, Wire_Type)
                    Sheet7.Visible = xlSheetVisible 'Checklist
                    Sheet4.Visible = xlSheetVisible 'Confirmation-Outgoing-1
                    Sheet2.Visible = xlSheetVisible 'Wire Transfer Request-1
            End Select
        Else
            Hide_All
            .Range("B6").Select
        End If
    End With
End Sub
Public Sub EntryB6()
    Hide_All
    With DATAENTRY
        If Not .Range("B6") = vbNullString Then
            Select Case IsNumeric(.Range("B6").Value2)
                Case Is = False
                    .Range("A300:A312").EntireRow.Hidden = False
                    .Range("A317:A331").EntireRow.Hidden = False
                    .Range("B301").Select
                    With ThisWorkbook
                        Sheet3.Visible = xlSheetVisible  'Checklist-Loan Closing
                        Sheet12.Visible = xlSheetVisible 'Confirmation-Outgoing-2
                        Sheet11.Visible = xlSheetVisible 'Wire Transfer Request-2
                    End With
                Case Is = True
                    CIFGrab CIFNumber:=.Range("B307").value, WireType:="OutGoingLoan"
                    '.Range("A218:A220,A222:A223").EntireRow.Hidden = False
                    .Range("A301:A312,A317:A319:A339").EntireRow.Hidden = False
                    .Range("C220") = RECURRINGOUTGOINGIDMETHOD
                    Unique_Identifier = .Range("B6").value
                    Wire_Type = "Loan Closing"
                    Call Find_Recurring(Unique_Identifier, Wire_Type)
                    Sheet7.Visible = xlSheetVisible 'Checklist
                    Sheet13.Visible = xlSheetVisible 'Wire Transfer Request - Brokered-Internet
            End Select
                .Range("B7").Select
        End If
    End With
End Sub
Public Sub EntryB7()
    Hide_All
    With DATAENTRY
        Select Case .Range("B7")
            Case Is <> ""
                .Range("A400:A412").EntireRow.Hidden = False
                .Range("A415:A499").EntireRow.Hidden = False
                .Range("B401").Select
                With ThisWorkbook
                    Sheet9.Visible = xlSheetVisible  'Checklist-Cash Management
                    Sheet14.Visible = xlSheetVisible 'Confirmation-Outgoing-3
                End With
            Case Else
                Range("B8").Select
        End Select
    End With
End Sub
Public Sub EntryB8()
    Hide_All
    With DATAENTRY
        If Not .Range("B8") = vbNullString Then
            Select Case IsNumeric(.Range("B8").Value2)
                Case Is = False
                    .Range("A500:A599").EntireRow.Hidden = False
                    .Range("B501").Select
                    With ThisWorkbook
                        Sheet13.Visible = xlSheetVisible 'Wire Transfer Request - Brokered-Internet
                    End With
                Case Else
                    .Range("A218:A220,A222:A223").EntireRow.Hidden = False
                    .Range("A501:A543").EntireRow.Hidden = False
                    .Range("C220") = RECURRINGOUTGOINGIDMETHOD
                    Unique_Identifier = .Range("B8").value
                    Wire_Type = "Brokered"
                    Call Find_Recurring(Unique_Identifier, Wire_Type)
                    Sheet7.Visible = xlSheetVisible 'Checklist
                    Sheet13.Visible = xlSheetVisible 'Wire Transfer Request - Brokered-Internet
                End Select
        Else
            Hide_All
            .Range("B9").Select
        End If
    End With
End Sub
Public Sub EntryB9()
    Hide_All
    With DATAENTRY
        If Not .Range("B9") = "" Then
            Select Case IsNumeric(.Range("B9"))
                 Case Is = False
                     .Range("A600:A610").EntireRow.Hidden = False
                     .Range("B601").Select
                     Sheet8.Visible = xlSheetVisible 'Checklist-Internal
                Case Is = True
                    .Range("A222,A223").EntireRow.Hidden = False
                    .Range("A600:A699").EntireRow.Hidden = False
                    Unique_Identifier = Range("B9").value
                    Wire_Type = "Internal"
                    Call Find_Recurring(Unique_Identifier, Wire_Type)
            End Select
        Else
            Hide_All
            Range("B10").Select
        End If
    End With
End Sub
Public Sub EntryB205()
    With DATAENTRY
        Select Case LCase$(.Range("B205"))
            Case Is = "yes"
                .Rows("212:215").Hidden = False
            Case Else
                .Rows("212:215").Hidden = True
                .Range("B206").Select
        End Select
    End With
End Sub
Public Sub EntryB227()
    With DATAENTRY
        Select Case LCase$(.Range("B227"))
            Case Is = "domestic"
                .Range("A222:A243").EntireRow.Hidden = False
                .Range("A267:A299").EntireRow.Hidden = False
                .Range("A244:A266").EntireRow.Hidden = True
                .Range("B229").Select
            Case Is = "international"
                .Range("A244:A299").EntireRow.Hidden = False
                .Range("A228:A243").EntireRow.Hidden = True
                .Range("B245").Select
            Case Is <> "international", "domestic"
                .Range("A228:A299").EntireRow.Hidden = True
                .Range("B227").Select
        End Select
    End With
End Sub
Public Sub EntryB269()
    With DATAENTRY
        Select Case LCase$(.Range("B269"))
            Case Is = "yes"
                Sheets("Wire Transfer Agreement").Visible = True
                .Range("A5000:A5099").EntireRow.Hidden = False
                .Range("B282:B299").EntireRow.Hidden = True
                Application.ScreenUpdating = True
                .Range("B5001").Select
            Case Else
                Sheets("Wire Transfer Agreement").Visible = False
                .Range("A5000:A5099").EntireRow.Hidden = True
                .Range("B281:B299").EntireRow.Hidden = False
                .Range("B270").Select
        End Select
    End With
End Sub
Public Sub EntryB306()
    With DATAENTRY
        Select Case LCase$(.Range("B306"))
            Case Is = "yes"
                .Range("A313:A316,A331").EntireRow.Hidden = False
            Case Else
                .Range("A313:A316").EntireRow.Hidden = True
                .Range("A331").EntireRow.Hidden = False
                .Range("B307").Select
        End Select
    End With
End Sub
Public Sub EntryB331()
    With DATAENTRY
        Select Case LCase$(.Range("B331"))
            Case Is = "domestic"
                .Range("A332:A347").EntireRow.Hidden = False
                .Range("A370:A399").EntireRow.Hidden = False
                .Range("A348:A369").EntireRow.Hidden = True
                .Range("B331").Select
            Case Is = "international"
                .Range("A347:A399").EntireRow.Hidden = False
                .Range("A332:A346").EntireRow.Hidden = True
                .Range("B349").Select
            Case Is <> "domestic", "international"
                .Range("A332:A399").EntireRow.Hidden = True
                .Range("B331").Select
        End Select
    End With
End Sub
Public Sub EntryB373()
    With DATAENTRY
        Select Case LCase$(.Range("B373"))
            Case Is = "yes"
                Sheets("Wire Transfer Agreement").Visible = True
                .Range("A5000:A5099").EntireRow.Hidden = False
                .Range("B383:B399").EntireRow.Hidden = True
                Application.ScreenUpdating = True
                .Range("B5001").Select
            Case Else
                Sheets("Wire Transfer Agreement").Visible = False
                .Range("A5000:A5099").EntireRow.Hidden = True
                .Range("B383:B399").EntireRow.Hidden = False
                .Range("B374").Select
        End Select
    End With
End Sub
Public Sub EntryB406()
    With DATAENTRY
        Select Case LCase$(.Range("B406"))
            Case Is = "yes"
                .Range("A412:A413").EntireRow.Hidden = False
            Case Else
                .Range("A412:A413").EntireRow.Hidden = True
                .Range("B407").Select
        End Select
    End With
End Sub
Public Sub EntryB425()
    With DATAENTRY
        Select Case LCase$(.Range("B425"))
             Case Is = "yes"
                 .Range("A430:A431").EntireRow.Hidden = False
             Case Else
                 .Range("A430:A431").EntireRow.Hidden = True
                 .Range("B426").Select
         End Select
    End With
End Sub
Public Sub EntryB610()
    With DATAENTRY
        Select Case LCase$(.Range("B610"))
            Case Is = "domestic"
                .Range("A611:A625").EntireRow.Hidden = False
                .Range("A648:A699").EntireRow.Hidden = False
                .Range("A626:A647").EntireRow.Hidden = True
                .Range("B612").Select
            Case Is = "international"
                .Range("A626:A699").EntireRow.Hidden = False
                .Range("A611:A625").EntireRow.Hidden = True
                .Range("B627").Select
            Case Is <> "international", "domestic"
                .Range("A611:A699").EntireRow.Hidden = True
                .Range("B610").Select
        End Select
    End With
End Sub
Public Sub EntryB5004()
    With DATAENTRY
        .Range("A5005:A5011").EntireRow.Hidden = True
        .Range("B5004").Select
        Select Case LCase$(.Range("B5004"))
            Case Is = "entity"
                .Range("A5007:A5011").EntireRow.Hidden = False
                .Range("B5007").Select
            Case Is = "individual(s)"
                .Range("A5005:A5006").EntireRow.Hidden = False
                .Range("B5005").Select
        End Select
    End With
End Sub
Public Sub EntryB5104()
    With DATAENTRY
        .Range("A5111:A5114").EntireRow.Hidden = True
        .Range("B5105").Select
        Select Case LCase$(.Range("B5104"))
            Case Is = "yes"
                .Range("A5111:A5114").EntireRow.Hidden = False
                .Range("B5105").Select
            Case Is = "no"
                .Range("A5111:A5114").EntireRow.Hidden = True
                .Range("B5105").Select
        End Select
    End With
End Sub
Public Sub EntryB5118()
    With DATAENTRY
        Select Case LCase$(.Range("B5118"))
            Case Is = "domestic"
                .Range("A5119:A5131").EntireRow.Hidden = False
                .Range("A5132:A5199").EntireRow.Hidden = True
                .Range("A5150").EntireRow.Hidden = False
                .Range("B5120").Select
            Case Is = "international"
                .Range("A5119:A5131").EntireRow.Hidden = True
                .Range("A5132:A5149").EntireRow.Hidden = False
                .Range("A5151:A5199").EntireRow.Hidden = True
                .Range("B5133").Select
            Case Is <> "international", "domestic"
                .Range("A5119:A5199").EntireRow.Hidden = True
                .Range("B5118").Select
        End Select
    End With
End Sub
Public Sub CreateAgreementRecurring(ByVal CreateNewAgreement As Boolean, ByVal CreateRecurringRequest As Boolean)
    If CreateNewAgreement And CreateRecurringRequest = False Then
        Sheet6.Visible = xlSheetVisible 'Wire Transfer Agreement
        With DATAENTRY
            .Range("A5000:A5099").EntireRow.Hidden = False
            '.Range("A5005:A5011").EntireRow.Hidden = True
            .Range("B5001").Select
        End With
    End If
    
    If CreateNewAgreement = False And CreateRecurringRequest Then
        Sheet18.Visible = xlSheetVisible 'Recurring Wire Transfer Request
        With DATAENTRY
            .Range("B218:B225").EntireRow.Hidden = False
            .Range("C220") = RECURRINGOUTGOINGIDMETHOD
            .Range("A5100:A5118").EntireRow.Hidden = False
            '.Range("A5111:A5114").EntireRow.Hidden = True
            .Range("A5087:A5099").EntireRow.Hidden = True
            .Range("B5101").Select
            Sheet7.Visible = xlSheetVisible 'Checklist
        End With
    End If
    
    If CreateNewAgreement And CreateRecurringRequest Then
        Sheet6.Visible = xlSheetVisible 'Wire Transfer Agreement
        Sheet18.Visible = xlSheetVisible 'Recurring Wire Transfer Request
        With DATAENTRY
            .Range("C220") = RECURRINGOUTGOINGIDMETHOD
            .Range("A5000:A5099").EntireRow.Hidden = False
            '.Range("A5005:A5011").EntireRow.Hidden = True
            .Range("B5001").Select
            .Range("A5100:A5118").EntireRow.Hidden = False
            '.Range("A5111:A5114").EntireRow.Hidden = True
            .Range("A5087:A5099").EntireRow.Hidden = True
            '.Range("B5101").Select
            Sheet7.Visible = xlSheetVisible 'Checklist
        End With
    End If
    
End Sub
Option Explicit
Public Sub CIFGrab(ByVal CIFNumber As String, WireType As String)
    Dim tDBGrabRecord As WireCustInfo
    
    Set tDBGrabRecord = getCIFDBGrabTestRecord(Array(BRANSON, CHARLOTTE), CIFNumber)
    
    If tDBGrabRecord Is Nothing Then
        MsgBox "Failed to get record", vbExclamation
    Else
        Select Case WireType
                Case Is = "Incoming"
                    With DATAENTRY
                        .Range("B104") = tDBGrabRecord.CustomerName
                        .Range("B105") = tDBGrabRecord.CustomerAddress
                        .Range("B107") = tDBGrabRecord.CustomerCityStateZip
                    End With
                Case Is = "OutGoingDDALoan"
                    With DATAENTRY
                        .Range("B207") = tDBGrabRecord.CustomerName
                        .Range("B208") = tDBGrabRecord.CustomerAddress
                        .Range("B209") = tDBGrabRecord.CustomerCityStateZip
                        If tDBGrabRecord.CustomerHomePhone = 0 Or tDBGrabRecord.CustomerHomePhone = "" Then
                            .Range("B210") = Format(tDBGrabRecord.CustomerCellPhone, "(###) ###-####")
                        ElseIf tDBGrabRecord.CustomerCellPhone = 0 Then
                            .Range("B210") = Format(tDBGrabRecord.CustomerHomePhone, "(###) ###-####")
                        ElseIf tDBGrabRecord.CustomerHomePhone = 0 And tDBGrabRecord.CustomerCellPhone = 0 Then
                            .Range("B210") = vbNullString
                        End If
                        .Range("B211") = tDBGrabRecord.CustomerBSA
                    End With
                Case Is = "OutGoingLoan"
                    With DATAENTRY
                        .Range("B308") = tDBGrabRecord.CustomerName
                        .Range("B309") = tDBGrabRecord.CustomerAddress
                        .Range("B310") = tDBGrabRecord.CustomerCityStateZip
                        If tDBGrabRecord.CustomerHomePhone = 0 Then
                            .Range("B311") = Format(tDBGrabRecord.CustomerCellPhone, "(###) ###-####")
                        ElseIf tDBGrabRecord.CustomerCellPhone = 0 Then
                            .Range("B311") = Format(tDBGrabRecord.CustomerHomePhone, "(###) ###-####")
                        ElseIf tDBGrabRecord.CustomerHomePhone = 0 And tDBGrabRecord.CustomerCellPhone = 0 Then
                            .Range("B311") = vbNullString
                        End If
                        .Range("B312") = tDBGrabRecord.CustomerBSA
                    End With
                Case Is = "OutGoingCM"
                    With DATAENTRY
                        .Range("B408") = tDBGrabRecord.CustomerName
                        .Range("B409") = tDBGrabRecord.CustomerAddress
                        .Range("B410") = tDBGrabRecord.CustomerCityStateZip
                        If tDBGrabRecord.CustomerHomePhone = 0 Then
                            .Range("B411") = Format(tDBGrabRecord.CustomerCellPhone, "(###) ###-####")
                        ElseIf tDBGrabRecord.CustomerCellPhone = 0 Then
                            .Range("B411") = Format(tDBGrabRecord.CustomerHomePhone, "(###) ###-####")
                        ElseIf tDBGrabRecord.CustomerHomePhone = 0 And tDBGrabRecord.CustomerCellPhone = 0 Then
                            .Range("B411") = vbNullString
                        End If
                        .Range("B412") = tDBGrabRecord.CustomerBSA
                    End With
                Case Is = "OutGoingBrokered"
                    With DATAENTRY
                        .Range("B507") = tDBGrabRecord.CustomerName
                        .Range("B508") = tDBGrabRecord.CustomerAddress
                        .Range("B509") = tDBGrabRecord.CustomerCityStateZip
                        If tDBGrabRecord.CustomerHomePhone = 0 Then
                            .Range("B510") = Format(tDBGrabRecord.CustomerCellPhone, "(###) ###-####")
                        ElseIf tDBGrabRecord.CustomerCellPhone = 0 Then
                            .Range("B510") = Format(tDBGrabRecord.CustomerHomePhone, "(###) ###-####")
                        ElseIf tDBGrabRecord.CustomerHomePhone = 0 And tDBGrabRecord.CustomerCellPhone = 0 Then
                            .Range("B510") = vbNullString
                        End If
                    End With
                Case Is = "Recurring"
                    With DATAENTRY
                        .Range("B5106") = tDBGrabRecord.CustomerName
                        .Range("B5107") = tDBGrabRecord.CustomerAddress
                        .Range("B5108") = tDBGrabRecord.CustomerCityStateZip
                        If tDBGrabRecord.CustomerHomePhone = 0 Then
                            .Range("B5109") = Format(tDBGrabRecord.CustomerCellPhone, "(###) ###-####")
                        ElseIf tDBGrabRecord.CustomerCellPhone = 0 Then
                            .Range("B5109") = Format(tDBGrabRecord.CustomerHomePhone, "(###) ###-####")
                        ElseIf tDBGrabRecord.CustomerHomePhone = 0 And tDBGrabRecord.CustomerCellPhone = 0 Then
                            .Range("B5109") = vbNullString
                        End If
                        .Range("B5110") = tDBGrabRecord.CustomerBSA
                    End With
            End Select
        'PopulateCIFBasedOnTheWireType wireTypeIs:=WireType
    End If
End Sub
Private Function getCIFDBGrabTestRecord(arrNames, ByVal CustNum As String) As WireCustInfo
    Dim conn As New ADODB.Connection
    Dim rs As New ADODB.Recordset
    Dim SQL As String, nm, okSQL As Boolean
    Dim tDBGrabRecord As WireCustInfo
    
    conn.Open CONNECTIONSTRING
    
    For Each nm In arrNames
        SQL = getCIFDBGrabSQL(CStr(nm), CustNum)
        
        On Error Resume Next
        rs.Open SQL, conn
        If Err.Number = 0 Then okSQL = True
        On Error GoTo 0
        
        If okSQL Then
            If Not rs.EOF Then
                Set tDBGrabRecord = New WireCustInfo
                With tDBGrabRecord
                    .cust_Name = Trim(rs.Fields(0).value)
                    .cust_Address = Trim(rs.Fields(1).value)
                    .cust_CityStateZip = Trim(rs.Fields(2).value)
                    .cust_HomePhone = Trim(rs.Fields(3).value)
                    .cust_CellPhone = Trim(rs.Fields(4).value)
                    .cust_BSA = Trim(rs.Fields(5).value)
                End With
            End If
            Exit For
        End If
    Next nm
    
    If rs.State = adStateOpen Then rs.Close
    If conn.State = adStateOpen Then conn.Close
    
    Set getCIFDBGrabTestRecord = tDBGrabRecord
    

End Function
Private Function getCIFDBGrabSQL(ByVal TableName As String, ByVal CIF As String) As String
    Dim SelectClause As String, _
        FromClause As String, _
        WhereClause As String

    SelectClause = GetSelectClause
    FromClause = "FROM " & TableName
    WhereClause = "WHERE cfcif# = '" & CIF & "'"
    
    getCIFDBGrabSQL = SelectClause & vbNewLine & FromClause & vbNewLine & WhereClause
    
    'Debug.Print getCIFDBGrabSQL
    
End Function
Private Function GetSelectClause() As String
    Const Delimiter As String = vbNewLine
    Dim list As Object
    Set list = CreateObject("System.Collections.ArrayList")
    
    With list
        .Add "SELECT cfna1,"
        .Add "COALESCE(NULLIF(RTRIM(LTRIM(cfpfa1))|| ' '|| RTRIM(LTRIM(cfpfa2)), ''),RTRIM(LTRIM(cfna2))|| ' ' || RTRIM(LTRIM(cfna3))),"
        .Add "RTRIM(LTRIM(cfcity)) || ', ' || RTRIM(LTRIM(cfstat)) || ', ' || RTRIM(LTRIM(LEFT(cfzip,5))),"
        .Add "cfhpho,"
        .Add "cfcel1,"
        .Add "cfudsc6"
    End With
    
    GetSelectClause = Join(list.ToArray, Delimiter)
End Function
Option Explicit
Public Sub Hide_All()
'Used a Loop through the worksheet code names instead of identifying the sheets individually.
'Helps use less memory and also if the "Sheet Name" is ever changed
'by a user the code wont break.


    Range("A12:A9999").EntireRow.Hidden = True
    
    Dim sh As Worksheet
    Dim i As Integer
    
    For i = 2 To 15
        For Each sh In ThisWorkbook.Worksheets
            If sh.CodeName = "Sheet" & i Then sh.Visible = xlSheetHidden
        Next sh
    Next i
    
    Sheet18.Visible = xlSheetHidden

End Sub
Public Sub Find_Recurring(ByVal Unique_Identifier As String, Wire_Type As String)

    Dim srcWB As Workbook, destWB As Workbook
    Dim srcWS As Worksheet, destWS As Worksheet
    Dim FoundCell As Range
    Dim Row As Long
    
    Set srcWB = Workbooks.Open(RECURRINGWORKBOOK)
    
    Select Case Wire_Type
        Case Is = "Deposit/Loan", "Brokered", "Loan Closing"
            Set srcWS = srcWB.Sheets("Recurring Requests")
        Case Is = "Internal"
            Set srcWS = srcWB.Sheets("Internal Requests")
    End Select
    
    Set destWB = ThisWorkbook
    Set destWS = destWB.Sheets("Data Entry")

    Set FoundCell = srcWS.Range("A:A").Find(What:=Unique_Identifier)
    
    If Not FoundCell Is Nothing Then
        Row = FoundCell.Row
        
        'Deposit account/loan account (post closing) (Cell B5)
        If Wire_Type = "Deposit/Loan" Then
            destWS.Range("A222:A243").EntireRow.Hidden = False
            destWS.Range("A267:A299").EntireRow.Hidden = False
            destWS.Range("A244:A266").EntireRow.Hidden = True
            destWS.Range("B206") = srcWS.Cells(Row, 5)   'CIF NUmber
            'destWS.Range("B507") = srcWS.Cells(Row, 6)   'Name
            'destWS.Range("B508") = srcWS.Cells(Row, 7)   'Address
            'destWS.Range("B509") = srcWS.Cells(Row, 8)   'City State Zip
            'destWS.Range("B510") = srcWS.Cells(Row, 9)   'Telephone Number
            destWS.Range("B216") = srcWS.Cells(Row, 15)  'Customer Account Number
            destWS.Range("B217") = srcWS.Cells(Row, 16)  'Account Number to Fund Wire
            destWS.Range("B227") = srcWS.Cells(Row, 17)  'Domestic/International
            Select Case LCase$(srcWS.Cells(Row, 17))
            'THIS HANDLES FINANCIAL INSTITUTION INFO FOR RECURRING INFO FOR DOMESTIC WIRES
                Case Is = "domestic"
                    destWS.Range("B229") = srcWS.Cells(Row, 19)  'Financial Institution Name
                    destWS.Range("B230") = srcWS.Cells(Row, 20)  'Routing ABA Number
                    destWS.Range("B231") = srcWS.Cells(Row, 21)  'Financial Institution Phone Number
                    destWS.Range("B232") = srcWS.Cells(Row, 22)  'Financial Institution Address
                    destWS.Range("B233") = srcWS.Cells(Row, 23)  'Beneficiary Name
                    destWS.Range("B234") = srcWS.Cells(Row, 24)  'Beneficiary Account Number
                    destWS.Range("B235") = srcWS.Cells(Row, 25)  'Beneficiary Physical Address
                    destWS.Range("B237") = srcWS.Cells(Row, 27)  'Intermediary Financial Institution Name
                    destWS.Range("B238") = srcWS.Cells(Row, 28)  'Intermediary Financial ABA/Routing Number
                    destWS.Range("B239") = srcWS.Cells(Row, 29)  'Intermediary Address
                    destWS.Range("B240") = srcWS.Cells(Row, 30)  'Intermediary Account Number
                Case Is = "international"
            End Select
            
            '*********THIS WILL ALL BE HANDLED IN THE CODE SEGMENT ABOVE*****
            'Hide domestic/international data-input rows as applicable
            'Select Case LCase$(destWS.Range("B227"))
            '    Case Is = "domestic"
            '        Range("A222:A243").EntireRow.Hidden = False
            '        Range("A267:A299").EntireRow.Hidden = False
            '        Range("A244:A266").EntireRow.Hidden = True
            '        Range("B201").Select
            '    Case Is = "international"
            '        Range("A244:A299").EntireRow.Hidden = False
            '        Range("A228:A243").EntireRow.Hidden = True
            '        Range("B201").Select
            '    Case Is <> "international", "domestic"
            '        Range("A228:A299").EntireRow.Hidden = True
            '        Range("B201").Select
            'End Select
            
        ElseIf Wire_Type = "Brokered" Then
        
            destWS.Range("B502:B533").ClearContents
        
            destWS.Range("B506") = srcWS.Cells(Row, 5)   'CIF NUmber
            'destWS.Range("B507") = srcWS.Cells(Row, 6)   'Name
            'destWS.Range("B508") = srcWS.Cells(Row, 7)   'Address
            'destWS.Range("B509") = srcWS.Cells(Row, 8)   'City State Zip
            'destWS.Range("B510") = srcWS.Cells(Row, 9)   'Telephone Number
            destWS.Range("B511") = srcWS.Cells(Row, 16)  'Account Number to Fund Wire
            destWS.Range("B514") = srcWS.Cells(Row, 19)  'Financial Institution Name
            destWS.Range("B515") = srcWS.Cells(Row, 20)  'Routing ABA Number
            destWS.Range("B516") = srcWS.Cells(Row, 21)  'Financial Institution Phone Number
            destWS.Range("B517") = srcWS.Cells(Row, 22)  'Financial Institution Address
            destWS.Range("B518") = srcWS.Cells(Row, 23)  'Beneficiary Name
            destWS.Range("B519") = srcWS.Cells(Row, 24)  'Beneficiary Account Number
            destWS.Range("B520") = srcWS.Cells(Row, 25)  'Beneficiary Physical Address
            destWS.Range("B522") = srcWS.Cells(Row, 27)  'Intermediary Financial Institution Name
            destWS.Range("B523") = srcWS.Cells(Row, 28)  'Intermediary Financial ABA/Routing Number
            destWS.Range("B524") = srcWS.Cells(Row, 29)  'Intermediary Financial ABA/Routing Number
            destWS.Range("B525") = srcWS.Cells(Row, 30)  'Intermediary Account Number
            
            srcWB.Close True
        
        ElseIf Wire_Type = "Loan Closing" Then
            destWS.Range("B322") = "Recipient is title company that is closing the sender's home purchase/refi."
            destWS.Range("B331") = "Domestic"
            destWS.Range("B333") = srcWS.Cells(Row, 19)  'Financial Institution Name
            destWS.Range("B334") = srcWS.Cells(Row, 20)  'Routing ABA Number
            destWS.Range("B336") = srcWS.Cells(Row, 22)  'Financial Institution Address
            destWS.Range("B337") = srcWS.Cells(Row, 23)  'Beneficiary Name
            destWS.Range("B338") = srcWS.Cells(Row, 24)  'Beneficiary Account Number
            destWS.Range("B339") = srcWS.Cells(Row, 25)  'Beneficiary Physical Address
            
            srcWB.Close True
            
        'NEED TO REFACTOR THIS LIKE THE CODE ABOVE YET
        'Internal Wire (Cell B9)
        ElseIf Wire_Type = "Internal" Then
        
            destWB.Activate
            destWS.Range("B601:B699").Select
            Selection.ClearContents
        
            srcWB.Activate
            srcWS.Activate
            srcWS.Range(srcWS.Cells(Row, 2), srcWS.Cells(Row, 6)).Select
            Selection.Copy
            destWB.Activate
            destWS.Range("B604").Select
            Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks _
                :=False, Transpose:=True

            srcWB.Activate
            srcWS.Range(srcWS.Cells(Row, 7), srcWS.Cells(Row, 7)).Select
            Selection.Copy
            destWB.Activate
            destWS.Range("B610").Select
            Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks _
                :=False, Transpose:=True

            srcWB.Activate
            srcWS.Range(srcWS.Cells(Row, 9), srcWS.Cells(Row, 20)).Select
            Selection.Copy
            destWB.Activate
            destWS.Range("B612").Select
            Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks _
                :=False, Transpose:=True

            srcWB.Activate
            srcWS.Range(srcWS.Cells(Row, 22), srcWS.Cells(Row, 38)).Select
            Selection.Copy
            destWB.Activate
            destWS.Range("B627").Select
            Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks _
                :=False, Transpose:=True

            srcWB.Activate
            srcWS.Range(srcWS.Cells(Row, 39), srcWS.Cells(Row, 40)).Select
            Selection.Copy
            destWB.Activate
            destWS.Range("B649").Select
            Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks _
                :=False, Transpose:=True

            Windows("Recurring Requests.xlsx").Activate
            ActiveWorkbook.Close
            
'            Windows("Wire Transfer Forms.xlsm").Activate
            
            'Hide domestic/international data-input rows as applicable
            Select Case LCase(Range("B610"))
                Case Is = "domestic"
                    Range("A611:A625").EntireRow.Hidden = False
                    Range("A648:A699").EntireRow.Hidden = False
                    Range("A626:A647").EntireRow.Hidden = True
                    Range("B612").Select
                Case Is = "international"
                    Range("A626:A699").EntireRow.Hidden = False
                    Range("A611:A625").EntireRow.Hidden = True
                    Range("B627").Select
                Case Is <> "international", "domestic"
                    Range("A611:A699").EntireRow.Hidden = True
                    Range("B610").Select
            End Select
        
        
        
    Else
        
        Windows("Recurring Requests.xlsx").Activate
        ActiveWorkbook.Close
    
    End If
    End If

End Sub
\$\endgroup\$
  • 1
    \$\begingroup\$ If all the variables in your WireCustInfo class are Public -- and you are not manipulating the values going in or out of the object -- then you don't need all the Get and Let properties for each variable. \$\endgroup\$ – PeterT Feb 21 at 15:50
  • \$\begingroup\$ @PeterT Thats good to know. I will remove the Get and Let Properties. \$\endgroup\$ – Zack E Feb 21 at 15:52
  • 1
    \$\begingroup\$ @PeterT suggestion should be: Make the property backing fieldsPrivate! Best practice seems to be Mathieu's this-pattern. @OP The Macro Recorder Curse can be cured ,) OOP Battleship Part 1: The Patterns may be a good start to VBA-OOP. Try the Rubberduck Add-In. \$\endgroup\$ – ComputerVersteher Feb 22 at 8:14
  • \$\begingroup\$ @ComputerVersteher I'm also good with all property backing fields being private - that is my own habit in fact. However there are some cases when I really just need a a structured object to hold some unmodified data that I will just keep it all Public. It's a philosophical debate over variables that do not need any checks or grooming if they should always be exposed via Property methods and I appreciate both sides of the argument. \$\endgroup\$ – PeterT Feb 22 at 20:12
  • \$\begingroup\$ @PeterT Maybe if you know what you're doing: But OOP novices like OP or me, not always know and a property is safe. Btw, what are your benefits? Less typing? A property explicit states his pürpose, a global var not. And you noticed the beautyness of thethisUDT? \$\endgroup\$ – ComputerVersteher Feb 22 at 21:03
14
+250
\$\begingroup\$

You include Option Explicit :+1 for that alone.


Range without any explicit worksheet implicitly is working off of whatever worksheet happens-to-be-active-at-the-time-that-code-is-executed. This will eventually blow up on you. Always qualify your Ranges with a worksheet IE Sheet1.Range("...") so there is no doubt as to what sheet you're referring to.


You have a typo in Public Property Let CustomerAddres(value As String). You're missing the second s


Within Public Sub EntryB5 you have Sheet7.Visible = xlSheetVisible 'Checklist. That comment is a signpost. That signpost says "Rename your worksheet objects so this comment can go away.". Display the Project Explorer from the View menu>Project Explorer (Hotkey: Ctrl+R), select Sheet7 from the Project Explorer and then displaying the Properties Window (Hotkey: F4). The first item in the properties window is (Name) which is actually the Worksheet.CodeName property. Name it something appropriate. I've used ChecklistWorksheet which changes your original code

Sheet7.Visible = xlSheetVisible 'Checklist

to

ChecklistWorksheet.Visible = xlSheetVisible

Absolutely no need for the comment any longer. Strive to make your code be self documenting. What the code is doing should immediately be apparent. If there's a need to describe why something is coded in a particular way that then warrants a comment. An example of this would be explaining why you're using CreateObject("System.Collections.ArrayList").


Public variables. I say stick them in a module that's named PublicVariables. Then whenever one of these variables is used, qualify it with the module name. Again in EntryB5 you have DATAENTRY

With DATAENTRY

when qualified with the module name you have

With PublicVariables.DATAENTRY

Now when future-you or another code (could be you 6 months from now) sees this line it's immediately apparent where this variable is coming from.


Module level variables. As was already mentioned in the comments the Rubberduck (RD) add-in has an inspection for this ModuleScopeDimKeyword

Private module variables should be declared using the 'Private' keyword. While 'Dim' is also legal, it should preferably be restricted to declarations of procedure-scoped local variables, for consistency, since public module variables are declared with the 'Public' keyword.

***Disclosure: I'm a contributing member. Just a tad biased in favor of it.


You already stated the names with underscores in them are inherited code you're working through. That being said _ are used as part of the Implements statement for implementing an interface and bringing it to your attention.

I saw a few parameters Wire_Type that use underscores. camelCase is typical for VBA parameters and local variables whereas PascalCase is used for Sub/Function (member) names. This would result in wireType as a replacement.


Your sub Hide_All has a loop within a loop. I prefer having the worksheet CodeNames in a table on a worksheet. That worksheet, whose CodeName was updated to HideSheetsLookup, has a ListObject containing the sheets you want to hide. It also has a function, shown below, that can be called to check whether a sheet should be hidden. The Me keyword returns a reference to the sheet that code is written for.

Public Function ShouldBeHidden(ByVal worksheetCodeName As String) As Boolean
    Dim foundCell As Range
    Set foundCell = Me.ListObjects("HideSheetTable").DataBodyRange.Find(What:=worksheetCodeName, LookAt:=XlLookAt.xlWhole)

    ShouldBeHidden = Not foundCell Is Nothing
End Function

That function is then called as below.

For Each sh In ThisWorkbook.Worksheets
    sh.Visible = Not HideSheetsLookup.ShouldBeHidden(sh.CodeName)
Next sh

This edit changes the original code from a quadratic O(n^2) to a linear O(n).

The table works off the Workheet.CodeName and also won't break if/when a user renames the Worksheet.Name.


Within EntryB7 and a few others you have a With statement that's not doing anything. Remove those with blocks.

With ThisWorkbook
    Sheet9.Visible = xlSheetVisible  'Checklist-Cash Management
    Sheet14.Visible = xlSheetVisible 'Confirmation-Outgoing-3
End With

Static cell ranges. PublicVariables.DATAENTRY.Range("B4") may not always be in cell B4. If you add a insert/delete a row above or a column to the left those cells will shift causing those static addresses to no longer do what you want/expect it to. Use named ranges instead because they'll shift with without issue.

Another benefit of this is it helps aid in code self documenting itself. "B4" has no meaning whereas a descriptive name like Range("CustomerName") is infinitely better.


Using Line continuation _ as part of a single Dim statement is a code smell. Dim each variable on its own line just before you use it. Because they are already within a dedicated function getCIFDBGrabSQL it won't matter having 3 extra lines.


Implicitly accessing default members. The code below is implicitly accessing the [_Default] member.

Dim foo As String
foo = Sheet1.Range("A1")

Fully qualify your member calls to make it explicitly clear what you're accessing. This way you unambiguously show what you want.

foo = Sheet1.Range("A1").Value2

Copy paste coding. CIFGrab has duplicated code for Case Is = "OutGoingLoan" and Case Is = "OutGoingCM". Possibly others but I'm not going to compare every single group. You're assigning values to ranges and by paramaterizing a Sub you reduce redundant code.

Private Sub AssignValuesTo(ByVal customer As WireCustInfo, _
                            ByVal customerName As Range, _
                            ByVal customerAddress As Range, _
                            ByVal customerCityStateZip As Range, _
                            ByVal customerPhoneNumber As Range, _
                            ByVal customerBSA As Range)
    customerName.Value2 = customer.customerName
    customerAddress.Value2 = customer.customerAddress
    customerCityStateZip.Value2 = customer.customerCityStateZip

    Const PhoneFormat As String = "(###) ###-####"
    If customer.customerHomePhone = 0 Then
        customerPhoneNumber.Value2 = Format(customer.customerCellPhone, PhoneFormat)
    ElseIf customer.customerCellPhone = 0 Then
        customerPhoneNumber.Value2 = Format(customer.customerHomePhone, PhoneFormat)
    Else
        customerPhoneNumber.Value2 = vbNullString
    End If

    customerBSA.Value2 = customer.customerBSA
End Sub

Now has a call site as below.

Case Is = "OutGoingLoan"
    With PublicVariables.DATAENTRY
        AssignValuesTo tDBGrabRecord, .Range("308"), .Range("309"), .Range("310"), .Range("311"), .Range("312")
    End With
Case Is = "OutGoingCM"
    With PublicVariables.DATAENTRY
        AssignValuesTo tDBGrabRecord, .Range("408"), .Range("409"), .Range("410"), .Range("411"), .Range("412")
    End With

Create dedicated Subs. The wire type that's Deposit or Loan

If Wire_Type = "Deposit/Loan" Then
    destWS.Range("A222:A243").EntireRow.Hidden = False
    destWS.Range("A267:A299").EntireRow.Hidden = False
    destWS.Range("A244:A266").EntireRow.Hidden = True
    destWS.Range("B206") = srcWS.Cells(Row, 5)   'CIF NUmber
    'destWS.Range("B507") = srcWS.Cells(Row, 6)   'Name
    'destWS.Range("B508") = srcWS.Cells(Row, 7)   'Address
    'destWS.Range("B509") = srcWS.Cells(Row, 8)   'City State Zip
    'destWS.Range("B510") = srcWS.Cells(Row, 9)   'Telephone Number
    destWS.Range("B216") = srcWS.Cells(Row, 15)  'Customer Account Number
    destWS.Range("B217") = srcWS.Cells(Row, 16)  'Account Number to Fund Wire
    destWS.Range("B227") = srcWS.Cells(Row, 17)  'Domestic/International
    Select Case LCase$(srcWS.Cells(Row, 17))
    'THIS HANDLES FINANCIAL INSTITUTION INFO FOR RECURRING INFO FOR DOMESTIC WIRES
        Case Is = "domestic"
            destWS.Range("B229") = srcWS.Cells(Row, 19)  'Financial Institution Name
            destWS.Range("B230") = srcWS.Cells(Row, 20)  'Routing ABA Number
            destWS.Range("B231") = srcWS.Cells(Row, 21)  'Financial Institution Phone Number
            destWS.Range("B232") = srcWS.Cells(Row, 22)  'Financial Institution Address
            destWS.Range("B233") = srcWS.Cells(Row, 23)  'Beneficiary Name
            destWS.Range("B234") = srcWS.Cells(Row, 24)  'Beneficiary Account Number
            destWS.Range("B235") = srcWS.Cells(Row, 25)  'Beneficiary Physical Address
            destWS.Range("B237") = srcWS.Cells(Row, 27)  'Intermediary Financial Institution Name
            destWS.Range("B238") = srcWS.Cells(Row, 28)  'Intermediary Financial ABA/Routing Number
            destWS.Range("B239") = srcWS.Cells(Row, 29)  'Intermediary Address
            destWS.Range("B240") = srcWS.Cells(Row, 30)  'Intermediary Account Number
        Case Is = "international"
    End Select

Can be refactored to something similar to what's below.

Private Sub DepositOrLoanWireType(ByVal hideEntireRowOfCells As Range, _
                                    ByVal displayEntireRowOfCells As Range, _
                                    ByVal destWS As Worksheet, _
                                    ByVal srcWS As Worksheet, _
                                    ByVal sourceRow As Long)
    displayEntireRowOfCells.EntireRow.Hidden = False
    hideEntireRowOfCells.EntireRow.Hidden = True

    'Figure out logic to supply ranges on destination worksheet
    destWS.Range("B206").Value2 = srcWS.Range("CIFNumber").Value2
    destWS.Range("B216").Value2 = srcWS.Range("CustomerAccountNumber").Value2
    destWS.Range("B217").Value2 = srcWS.Range("AccountNumberToFundWire").Value2
    destWS.Range("B227").Value2 = srcWS.Range("DomesticOrInternational").Value2

    'THIS HANDLES FINANCIAL INSTITUTION INFO FOR RECURRING INFO FOR DOMESTIC WIRES
    If LCase$(srcWS.Cells(sourceRow, 17).value) = "domestic" Then
        destWS.Range("B229").Value2 = srcWS.Range("FinancialInstution").Value2
        destWS.Range("B230").Value2 = srcWS.Range("RoutingABANumber").Value2
        destWS.Range("B231").Value2 = srcWS.Range("FinancialInstitutionPhoneNumber").Value2
        destWS.Range("B232").Value2 = srcWS.Range("FinancialInstitutionAddress")
        destWS.Range("B233").Value2 = srcWS.Range("BeneficiaryName").Value2
        destWS.Range("B234").Value2 = srcWS.Range("eneficiaryAccountNumber").Value2
        destWS.Range("B235").Value2 = srcWS.Range("BeneficiaryPhysicalAddress").Value2
        destWS.Range("B237").Value2 = srcWS.Range("IntermediaryFinancialInstitutionName").Value2
        destWS.Range("B238").Value2 = srcWS.Range("IntermediaryFinancialABARoutingNumber").Value2
        destWS.Range("B239").Value2 = srcWS.Range("IntermediaryAddress").Value2
        destWS.Range("B240").Value2 = srcWS.Range("IntermediaryAccountNumber").Value2
    End If
End Sub

Which is now called as below.

If wireType = "Deposit/Loan" Then
    DepositOrLoanWireType destWS.Range("A244:A266"), _
                            Union(destWS.Range("A222:A243"), destWS.Range("A267:A299")), _
                            destWS, _
                            srcWS, _
                            foundCell.Row

By repeating this process you end up with a much higher viewpoint (abstraction level) when looking at the code. You care about what it's doing, not how it's being done. Naming these Subs appropriately will do this for you. You won't be getting lost in any details as you review the main high level idea. If you do need to know how things are being done, then you can look into those subs.


Within InternalWireType you have Foo.Select immediately followed by Selection.Bar. You rarely need to actually select whatever it is. Shorten it by putting them directly together.

destWS.Range("B601:B699").Select
Selection.ClearContents

shortens to

destWS.Range("B601:B699").ClearContents

The same goes for

srcWS.Range(srcWS.Cells(sourceRow, 2), srcWS.Cells(sourceRow, 6)).Copy
destWB.Activate
destWS.Range("B604").Select
Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks:=False, Transpose:=True

which becomes

srcWS.Range(srcWS.Cells(sourceRow, 2), srcWS.Cells(sourceRow, 6)).Copy
destWS.Range("B604").PasteSpecial Paste:=xlPasteValues, Transpose:=True

The use of a private helper type as a backing field for classes simplifies things, a lot. Below is how I implemented it and greatly simplifies things. Use properties even if they do nothing other than grab the backing field. It may seem like a redundant process but once you need validation logic before assigning to the backing field or retrieving it's value then you'll be glad you did.

Option Explicit

Private Type THelper
    Name As String
    Address As String
    CityStateZip As String
    Zip As String
    HomePhone As String
    CellPhone As String
    Phone As String
    BSA As String
    TableName As String
    ErrNumber As Long
End Type

Private this As THelper

Public Property Get Name() As String
    Name = this.Name
End Property

Public Property Let Name(value As String)
    Name = value
End Property

Public Property Get Address() As String
    Address = this.Address
End Property

Public Property Let Address(value As String)
    Address = value
End Property

Public Property Get CityStateZip() As String
    CityStateZip = this.CityStateZip
End Property

Public Property Let CityStateZip(value As String)
    CityStateZip = value
End Property

Public Property Get Zip() As String
    Zip = this.Zip
End Property

Public Property Let Zip(value As String)
    Zip = value
End Property

Public Property Get HomePhone() As String
    HomePhone = this.HomePhone
End Property

Public Property Let HomePhone(value As String)
    HomePhone = value
End Property

Public Property Get CellPhone() As String
    CellPhone = this.CellPhone
End Property

Public Property Let CellPhone(value As String)
    CellPhone = value
End Property

Public Property Get BSA() As String
    BSA = this.BSA
End Property

Public Property Let BSA(value As String)
    BSA = value
End Property

There's more that can be done but this enough for now.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you. I have a lot of reading and research to do based on this answer, but your explanations really help. \$\endgroup\$ – Zack E Feb 24 at 18:17
  • 1
    \$\begingroup\$ Minor note; be consistent in use of Value/Value2: If LCase$(srcWS.Cells(sourceRow, 17).value) = "domestic" Then destWS.Range("B229").Value2 = srcWS.Range("FinancialInstution").Value2; for this I'd probably use Value all over because it won't make a difference execution-wise and it's perhaps marginally more self documenting than having to justify use of Value2. But that's nothing, fab answer \$\endgroup\$ – Greedo Feb 24 at 23:37
  • \$\begingroup\$ Quick question. Where did HideSheetsLookup in sh.Visible = Not HideSheetsLookup.ShouldBeHidden(sh.CodeName) come from? \$\endgroup\$ – Zack E Feb 25 at 14:52
  • \$\begingroup\$ HideSheetsLookup is the CodeName for a Worksheet object that has the Table (ListObject) which contains all the CodeNames in the workbook you want to have hidden. Edited my answer to be clearer. \$\endgroup\$ – IvenBach Feb 26 at 20:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.