3
\$\begingroup\$

I inherited this code and have to fix it. It does work and I know I can refactor the code using If Not Intersect(Target, Me.Range()) Is Nothing Then syntax. I am wondering if using a function to pass the cell references in this case would be best, but im not really familiar on working with functions yet and would like some input or guidance on best practice with the code below. Please note I am well aware of the usage of select within this code block, but the original author wants me to keep the select to move the active cell based on selections made in the worksheet.

Private Sub Worksheet_Change(ByVal Target As Range)

    Application.ScreenUpdating = False
    Application.EnableEvents = False

    Dim wb As Workbook: Set wb = Application.ThisWorkbook
    Dim wsDE As Worksheet: Set wsDE = wb.Sheets("Data Entry")

    Dim Unique_Identifier As String
    Dim Wire_Type As String

    With wsDE
        Select Case Target.Address
            Case Is = "$B$4": Hide_All
                Select Case Range("B4")
                    Case Is <> ""
                        Range("A100:A199").EntireRow.Hidden = False
                        Range("B101").Select
                        Sheet5.Visible = xlSheetVisible 'Confirmation-Incoming
                        Range("B5") = ""
                    Case Else: Range("B5").Select
                End Select
            Case Is = "$B$5": Hide_All
                Select Case Range("B5")
                    Case Is <> ""
                        Range("A200:A211").EntireRow.Hidden = False
                        Range("A216:A227").EntireRow.Hidden = False
                        Range("B201").Select
                        With ThisWorkbook
                            Sheet7.Visible = xlSheetVisible 'Checklist
                            Sheet4.Visible = xlSheetVisible 'Confirmation-Outgoing-1
                            Sheet2.Visible = xlSheetVisible 'Wire Transfer Request-1
                        End With

                        Select Case Range("B5")
                            Case Is > 1
                                Range("A200:A299").EntireRow.Hidden = False
                                Unique_Identifier = Range("B5").Value
                                Wire_Type = "Deposit/Loan"
                                Call Find_Recurring(Unique_Identifier, Wire_Type)
                        End Select

                    Case Else: Range("B6").Select
                End Select
            Case Is = "$B$6": Hide_All
                Select Case Range("B6")
                    Case Is <> ""
                        Range("A300:A312").EntireRow.Hidden = False
                        Range("A316:A330").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 Else: Range("B7").Select
                End Select
            Case Is = "$B$7": Hide_All
                Select Case Range("B7")
                    Case Is <> ""
                        Range("A400:A411").EntireRow.Hidden = False
                        Range("A414: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
            Case Is = "$B$8": Hide_All
                Select Case Range("B8")
                    Case Is <> ""
                        Range("A500:A599").EntireRow.Hidden = False
                        Range("B501").Select
                        With ThisWorkbook
                            Sheet13.Visible = xlSheetVisible 'Wire Transfer Request - Brokered-Internet
                        End With
                    Case Else: Range("B9").Select
                End Select
            Case Is = "$B$9": Hide_All
                Select Case Range("B9")
                    Case Is <> ""
                        Range("A600:A610").EntireRow.Hidden = False
                        Range("B601").Select
                        Sheet8.Visible = xlSheetVisible 'Checklist-Internal

                        Select Case Range("B9")
                            Case Is > 1
                                Range("A600:A699").EntireRow.Hidden = False
                                Unique_Identifier = Range("B9").Value
                                Wire_Type = "Internal"
                                Call Find_Recurring(Unique_Identifier, Wire_Type)
                        End Select

                    Case Else: Range("B10").Select
                End Select
            Case Is = "$B$10": Hide_All
                Select Case Range("B10")
                    Case Is <> ""
                        Sheet6.Visible = xlSheetVisible 'Wire Transfer Agreement
                        Sheets("Wire Transfer Agreement").Visible = True
                        Range("A5000:A5099").EntireRow.Hidden = False
                        Range("A5005:A5011").EntireRow.Hidden = True
                        Range("B5001").Select
                    Case Else: Range("B11").Select
                End Select
            Case Is = "$B$11": Hide_All
                Select Case Range("B11")
                    Case Is <> ""
'                        Sheets("Recurring Wire Transfer Request").Visible = True
                        Sheet18.Visible = xlSheetVisible 'Recurring Wire Transfer Request
                        Range("A5100:A5118").EntireRow.Hidden = False
                        Range("A5111:A5114").EntireRow.Hidden = True
                        Range("B5101").Select
                    Case Else: Range("B11").Select
                End Select


    'Wires from Deposit Account or Loan (Post-Closing) Section
            Case Is = "$B$205"
                Select Case LCase(Range("B205"))
                    Case Is = "yes"
                        Range("A212:A215").EntireRow.Hidden = False
                    Case Else
                        Range("A212:A215").EntireRow.Hidden = True
                        Range("B206").Select
                End Select
            Case Is = "$B$227"
                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
            Case Is = "$B$269"
                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

    'Loan-Closing Wires Section
            Case Is = "$B$306"
                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
            Case Is = "$B$331"
                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
            Case Is = "$B$373"
                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

    'Cash Management Wires Section
            Case Is = "$B$406"
                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

            Case Is = "$B$425"
                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


    'Internal Foresight Wires Section
            Case Is = "$B$610"
                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

    'Wire Transfer Agreement Section
            Case Is = "$B$5004"
                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

    'Recurring Wire Transfer Request Section

            Case Is = "$B$5104"
                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

            Case Is = "$B$5118"
                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 Select

    End With



'CIF Calls
    If Not Intersect(Target, Range("B103")) Is Nothing Then CIFIncoming
    If Not Intersect(Target, Range("B206")) Is Nothing Then CIFOutD
    If Not Intersect(Target, Range("B307")) Is Nothing Then CIFOutL
    If Not Intersect(Target, Range("B407")) Is Nothing Then CIFOutCM
    If Not Intersect(Target, Range("B506")) Is Nothing Then CIFBrokered

    Application.ScreenUpdating = True
    Application.EnableEvents = True


End Sub
\$\endgroup\$
4
  • 1
    \$\begingroup\$ What do you have to "fix" and what is your point in refactoring? \$\endgroup\$ Commented Nov 26, 2019 at 14:22
  • \$\begingroup\$ What's going on with B5? You have two switches with the same criteria, one nested in the other. Looks like you could pull the second select case range("b5") out and it would work just fine. Have you attempted to simply some of the hidden ranges? E.g., Switch cases are for the cell and set a range, if range exists then .hidden=false. Lots of little things, but regarding the inquiry to the use of a Function(), i don't believe it would be appropriate to use when Target is literally the cell and adding the function would be redundant. \$\endgroup\$ Commented Nov 26, 2019 at 14:23
  • \$\begingroup\$ @PeterT I should have used the term "clean up" not fix, sorry about that. \$\endgroup\$ Commented Nov 26, 2019 at 14:26
  • \$\begingroup\$ @Cyril I have already started cleaning up the redundancies with the cell references :). Thanks for the input regarding the use of a Function() I can see what you are saying. \$\endgroup\$ Commented Nov 26, 2019 at 14:28

2 Answers 2

Reset to default
2
\$\begingroup\$

As AJD pointed out, using Named Ranges will make the code easier to understand, read, write and modify. The same logic can be applied to worksheet code names.

Here are the names that I used when refactoring the code:

  • Sheets("Wire Transfer Agreement") -> wsWTA
  • Sheet2 -> wsWireTransferRequest1
  • Sheet3 -> wsChecklistLoanClosing
  • Sheet4 -> wsConfirmationOutgoing1
  • Sheet5 -> wsConfirmationIncoming
  • Sheet7 -> wsChecklist
  • Sheet6 -> wsWireTransferAgreement
  • Sheet8 -> wsChecklistInternal
  • Sheet9 -> wsChecklistCashManagement
  • Sheet11 -> wsWireTransferRequest2
  • Sheet12 -> wsConfirmationOutgoing2
  • Sheet13 -> wsWTRBrokeredInternet
  • Sheet14 -> wsConfirmationOutgoing3
  • Sheet18 -> wsRecurringWTR

Using nested Select statements are particularly hard to read. Normally I would alternate Select with If..ElseIf..Else statements but the procedure is entirely too long; so I recommend writing a subroutine for each Case of the top level Select statement (see code below).

I only use Range.EntireRow when working with Range variables (e.g. Target.EntireRow). Using Rows() directly will make you code more condensed and the extra whitespace will make it easier to read.

Before

Range("A200:A211").EntireRow.Hidden = False

After

Rows("200:211").Hidden = False

Application.ScreenUpdating = True is no longer required. ScreenUpdating now resumes after the code has finished executing.

Refactored Code

Private Sub Worksheet_Change(ByVal Target As Range)
    Application.ScreenUpdating = False
    Application.EnableEvents = False
    Dim Unique_Identifier As String
    Dim Wire_Type As String

    Select Case Target.Address
        Case Is = "$B$4"
            EntryB4
        Case Is = "$B$5"
            EntryB5
        Case Is = "$B$6"
            EntryB6
        Case Is = "$B$7"
            EntryB7
        Case Is = "$B$8"
            EntryB8
        Case Is = "$B$9"
            EntryB9
        Case Is = "$B$10"
            EntryB10
        Case Is = "$B$11"
            EntryB11
            Rem Wires from Deposit Account or Loan (Post-Closing) Section
        Case Is = "$B$205"
            EntryB205
        Case Is = "$B$227"
            EntryB227
        Case Is = "$B$269"
            EntryB269
            Rem Loan-Closing Wires Section
        Case Is = "$B$306"
            EntryB306
        Case Is = "$B$331"
            EntryB331
        Case Is = "$B$373"
            EntryB373
            Rem Cash Management Wires Section
        Case Is = "$B$406"
            EntryB406
        Case Is = "$B$425"
            EntryB425
            Rem Internal Foresight Wires Section
        Case Is = "$B$610"
            EntryB610
            Rem Wire Transfer Agreement Section
        Case Is = "$B$5004"
            EntryB5004
            Rem Recurring Wire Transfer Request Section
        Case Is = "$B$5104"
            EntryB5104
        Case Is = "$B$5118"
            EntryB5118
    End Select

    Rem CIF Calls
    If Not Intersect(Target, Range("B103")) Is Nothing Then CIFIncoming
    If Not Intersect(Target, Range("B206")) Is Nothing Then CIFOutD
    If Not Intersect(Target, Range("B307")) Is Nothing Then CIFOutL
    If Not Intersect(Target, Range("B407")) Is Nothing Then CIFOutCM
    If Not Intersect(Target, Range("B506")) Is Nothing Then CIFBrokered

    Application.EnableEvents = True

End Sub

Sub EntryB4()
    With wsDataEntry
        .Activate
        Hide_All
        Select Case .Range("B4")
            Case Is <> ""
                .Rows("100:199").Hidden = False
                .Range("B101").Select
                wsConfirmationIncoming.Visible = xlSheetVisible
                .Range("B5") = ""
            Case Else
                .Range("B5").Select
        End Select
    End With
End Sub

Sub EntryB5()
    With wsDataEntry
        Hide_All
        .Activate
        Select Case .Range("B5")
            Case Is <> ""
                .Rows("200:211").Hidden = False
                .Rows("216:227").Hidden = False
                .Range("B201").Select
                With ThisWorkbook
                    wsChecklist.Visible = xlSheetVisible
                    wsConfirmationOutgoing1.Visible = xlSheetVisible
                    wsWireTransferRequest1.Visible = xlSheetVisible
                End With

                Select Case .Range("B5")
                    Case Is > 1
                        .Rows("200:299").Hidden = False
                        Unique_Identifier = .Range("B5").Value
                        Wire_Type = "Deposit/Loan"
                        Call Find_Recurring(Unique_Identifier, Wire_Type)
                End Select

            Case Else: .Range("B6").Select
        End Select
    End With
End Sub

Sub EntryB7()
    With wsDataEntry
        Hide_All
        .Activate
        Select Case .Range("B7")
            Case Is <> ""
                .Rows("400:411").Hidden = False
                .Rows("414:499").Hidden = False
                .Range("B401").Select
                With ThisWorkbook
                    wsChecklistCashManagement.Visible = xlSheetVisible
                    wsConfirmationOutgoing3.Visible = xlSheetVisible
                End With
            Case Else: .Range("B8").Select
        End Select
    End With
End Sub

Sub EntryB8()
    With wsDataEntry
        Hide_All
        .Activate
        Select Case .Range("B8")
            Case Is <> ""
                .Rows("500:599").Hidden = False
                .Range("B501").Select
                With ThisWorkbook
                    wsWTRBrokeredInternet.Visible = xlSheetVisible
                End With
            Case Else: .Range("B9").Select
        End Select
    End With
End Sub

Sub EntryB9()
    With wsDataEntry
        Hide_All
        .Activate
        Select Case .Range("B9")
            Case Is <> ""
                .Rows("600:610").Hidden = False
                .Range("B601").Select
                wsChecklistInternal.Visible = xlSheetVisible

                Select Case .Range("B9")
                    Case Is > 1
                        .Rows("600:699").Hidden = False
                        Unique_Identifier = .Range("B9").Value
                        Wire_Type = "Internal"
                        Call Find_Recurring(Unique_Identifier, Wire_Type)
                End Select

            Case Else: .Range("B10").Select
        End Select
    End With
End Sub

Sub EntryB10()
    With wsDataEntry
        Hide_All
        .Activate
        Select Case .Range("B10")
            Case Is <> ""
                Sheet6.Visible = xlSheetVisible
                wsWTA.Visible = True
                .Rows("5000:5099").Hidden = False
                .Rows("5005:5011").Hidden = True
                .Range("B5001").Select
            Case Else: .Range("B11").Select
        End Select
    End With
End Sub

Sub EntryB11()
    With wsDataEntry
        Hide_All
        .Activate
        Select Case .Range("B11")
            Case Is <> ""
                wsRecurringWTR.Visible = xlSheetVisible
                .Rows("5100:5118").Hidden = False
                .Rows("5111:5114").Hidden = True
                .Range("B5101").Select
            Case Else: .Range("B11").Select
        End Select
    End With
End Sub

Rem Wires from Deposit Account or Loan (Post-Closing) Section
Sub EntryB205()
    With wsDataEntry
        .Activate
        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

Sub EntryB227()
    With wsDataEntry
        .Activate
        Select Case LCase(.Range("B227"))
            Case Is = "domestic"
                .Rows("222:243").Hidden = False
                .Rows("267:299").Hidden = False
                .Rows("244:266").Hidden = True
                .Range("B229").Select
            Case Is = "international"
                .Rows("244:299").Hidden = False
                .Rows("228:243").Hidden = True
                .Range("B245").Select
            Case Is <> "international", "domestic"
                .Rows("228:299").Hidden = True
                .Range("B227").Select
        End Select
    End With
End Sub

Sub EntryB269()
    With wsDataEntry
        .Activate
        Select Case LCase(.Range("B269"))
            Case Is = "yes"
                wsWTA.Visible = True
                .Rows("5000:5099").Hidden = False
                .Rows("282:299").Hidden = True

                .Range("B5001").Select
            Case Else
                wsWTA.Visible = False
                .Rows("5000:5099").Hidden = True
                .Rows("281:299").Hidden = False
                .Range("B270").Select
        End Select
    End With
End Sub

Rem Loan-Closing Wires Section
Sub EntryB306()
    With wsDataEntry
        .Activate
        Select Case LCase(.Range("B306"))
            Case Is = "yes"
                .Range("A313:A316,A331").EntireRow.Hidden = False
            Case Else
                .Rows("313:316").Hidden = True
                .Rows(331).Hidden = False
                .Range("B307").Select
        End Select
    End With
End Sub

Sub EntryB331()
    With wsDataEntry
        .Activate
        Select Case LCase(.Range("B331"))
            Case Is = "domestic"
                .Rows("332:347").Hidden = False
                .Rows("370:399").Hidden = False
                .Rows("348:369").Hidden = True
                .Range("B331").Select
            Case Is = "international"
                .Rows("347:399").Hidden = False
                .Rows("332:346").Hidden = True
                .Range("B349").Select
            Case Is <> "domestic", "international"
                .Rows("332:399").Hidden = True
                .Range("B331").Select
        End Select
    End With
End Sub

Sub EntryB373()
    With wsDataEntry
        .Activate
        Select Case LCase(.Range("B373"))
            Case Is = "yes"
                wsWTA.Visible = True
                .Rows("5000:5099").Hidden = False
                .Rows("383:399").Hidden = True

                .Range("B5001").Select
            Case Else
                wsWTA.Visible = False
                .Rows("5000:5099").Hidden = True
                .Rows("383:399").Hidden = False
                .Range("B374").Select
        End Select

    End With
End Sub

Rem Cash Management Wires Section
Sub EntryB406()
    With wsDataEntry
        .Activate
        Select Case LCase(.Range("B406"))
            Case Is = "yes"
                .Rows("412:413").Hidden = False
            Case Else
                .Rows("412:413").Hidden = True
                .Range("B407").Select
        End Select
    End With
End Sub

Sub EntryB425()
    With wsDataEntry
        .Activate
        Select Case LCase(.Range("B425"))
            Case Is = "yes"
                .Rows("430:431").Hidden = False
            Case Else
                .Rows("430:431").Hidden = True
                .Range("B426").Select
        End Select
    End With
End Sub

Rem Internal Foresight Wires Section
Sub EntryB610()
    With wsDataEntry
        .Activate
        Select Case LCase(.Range("B610"))
            Case Is = "domestic"
                .Rows("611:625").Hidden = False
                .Rows("648:699").Hidden = False
                .Rows("626:647").Hidden = True
                .Range("B612").Select
            Case Is = "international"
                .Rows("626:699").Hidden = False
                .Rows("611:625").Hidden = True
                .Range("B627").Select
            Case Is <> "international", "domestic"
                .Rows("611:699").Hidden = True
                .Range("B610").Select
        End Select

    End With
End Sub

Rem Wire Transfer Agreement Section
Sub EntryB5004()
    With wsDataEntry
        .Activate
        .Rows("5005:5011").Hidden = True
        .Range("B5004").Select
        Select Case LCase(.Range("B5004"))
            Case Is = "entity"
                .Rows("5007:5011").Hidden = False
                .Range("B5007").Select
            Case Is = "individual(s)"
                .Rows("5005:5006").Hidden = False
                .Range("B5005").Select
        End Select
    End With
End Sub

Rem Recurring Wire Transfer Request Section
Sub EntryB5104()
    With wsDataEntry
        .Activate
        .Rows("5111:5114").Hidden = True
        .Range("B5105").Select
        Select Case LCase(.Range("B5104"))
            Case Is = "yes"
                .Rows("5111:5114").Hidden = False
                .Range("B5105").Select
            Case Is = "no"
                .Rows("5111:5114").Hidden = True
                .Range("B5105").Select
        End Select
    End With
End Sub

Sub EntryB5118()
    With wsDataEntry
        .Activate
        Select Case LCase(.Range("B5118"))
            Case Is = "domestic"
                .Rows("5119:5131").Hidden = False
                .Rows("5132:5199").Hidden = True
                .Rows(5150).Hidden = False
                .Range("B5120").Select
            Case Is = "international"
                .Rows("5119:5131").Hidden = True
                .Rows("5132:5149").Hidden = False
                .Rows("5151:5199").Hidden = True
                .Range("B5133").Select
            Case Is <> "international", "domestic"
                .Rows("5119:5199").Hidden = True
                .Range("B5118").Select
        End Select
    End With
End Sub
\$\endgroup\$
3
  • \$\begingroup\$ It's awesome that you refactored the code using almost the same method I came up with. I did use a function for the domestic and international because I wanted to try my hand at creating a function. \$\endgroup\$ Commented Nov 27, 2019 at 12:32
  • \$\begingroup\$ @ZackE Thanks for accepting my answer! \$\endgroup\$ Commented Nov 27, 2019 at 16:29
  • \$\begingroup\$ They were both great answers. I wish I could have accepted them both but your answer was inline with what I did to refactor the code myself outside a couple of additions i wanted to make. :) \$\endgroup\$ Commented Nov 27, 2019 at 16:32
5
\$\begingroup\$

I think this is what, in rugby, they would call a 'hospital pass'.

As someone who has fixed much code like this, I am going to hit some highlights only. If you manage to fix these highlights, I would love to see the revised code in another question for the second round. Because the job you have undertaken will take many passes to get right (but it will be worth it).

Set yourself up for success

[…] but the original author wants me to keep the select [...]

If you are fixing and maintaining the code, then it must be written in a way that makes it easy for you to maintain. If you are fixing this and someone else must maintain it, then be that nice coder and make it easy for them to maintain. However, moving the user to view particular cells in a work process is a user requirement that you should keep in mind.

Option Explicit at the top of every module. Just a reminder - you might already have it there.

Use Named Ranges in the sheets. It will make future maintenance so much easier. And it will make the current code easier to understand - .Range("DateEntryDate") is easier to understand than .Range("$B$3")

Exit early

For performance reasons alone, always find the reasons not to run the code at the very start and exit. At the moment, if I make a change in $ABC$678023983, this code is going to fire and run. What a waste of time and cycles! Example:

Private Sub Worksheet_Change(ByVal Target As Range)
Dim validRange as Range
    Set validRange = Intersect(Target, Me.Range("$B4:$B11")) '<-- wow, a named range here would be good.
    If validRange is Nothing Then
        Exit Sub ' <--- explicit exit, easy to see.
    End If

    If Target.Cells.Count > 1 Then
        Exit Sub ' <-- always manage the range size!
    End If

    ' … other code 
End Sub

Qualify your ranges

Most of the code is written with unqualified ranges (implied action on the active sheet).

Range("A400:A411").EntireRow.Hidden = False

However, this assumes that the active sheet continues to be the sheet that the _Change event occurred in. Never make that assumption. Remember this code?

Range("B101").Select

This means that the active cell will jump. With future modifications to the code or workbook, this may even jump to a different sheet.

In addition, the code calls some utility functions (e.g. Hide_all) - these may also alter the active sheet.

Having noted that, what is with With wsDE? There is an entire With block that in no way whatsoever that references the object (wsDE)!

Code readability

Don't use the line joiner, it can lead to confusion:

        Case Is = "$B$4": Hide_All
            Select Case Range("B4")
                Case Is <> ""
                    Range("A100:A199").EntireRow.Hidden = False
                    Range("B101").Select
                    Sheet5.Visible = xlSheetVisible 'Confirmation-Incoming
                    Range("B5") = ""
                Case Else: Range("B5").Select
            End Select

Should be:

        Case Is = "$B$4"
            Hide_All
            Select Case Range("B4")
                Case Is <> ""
                    Range("A100:A199").EntireRow.Hidden = False
                    Range("B101").Select
                    Sheet5.Visible = xlSheetVisible 'Confirmation-Incoming
                    Range("B5") = ""
                Case Else
                    Range("B5").Select
            End Select

All of a sudden, the indent levels and code scope is easier to understand. The Else becomes more obvious. Much easier to read.

Declare variables closer to where you are going to use them, and in the same scope. 

Dim Unique_Identifier As String
Dim Wire_Type As String

Took me a while to work out if they were actually used.

What is next?

If you address the above points you will end up with some slightly cleaner code. You will recognise yourself that it requires more work. However, you will have a cleaner foundation to figure out the remaining inefficiencies. One step at a time and you will get there!

\$\endgroup\$
3
  • \$\begingroup\$ I appreciate you taking the time to look through all of this code. Those are the exact changes I made except for adding the Exit Sub since i didnt even think about that. I will post a round 2 of this once ive had a chance to go back and complete the updating I've been working on :). \$\endgroup\$ Commented Nov 26, 2019 at 20:56
  • \$\begingroup\$ I hope you dont take offense that I accepted TinMan's answer. I only accepted his answer because it was almost the exact same way i had refactored the original code myself. I did upvote this answer because it did provide useful information not only for myself, but for others that may view this question. \$\endgroup\$ Commented Nov 27, 2019 at 14:04
  • \$\begingroup\$ no problems, glad you found them both useful. \$\endgroup\$ Commented Nov 27, 2019 at 18:41

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.