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 = "$B4ドル": 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 = "$B5ドル": 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 = "$B6ドル": 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 = "$B7ドル": 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 = "$B8ドル": 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 = "$B9ドル": 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 = "$B10ドル": 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 = "$B11ドル": 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 = "$B205ドル"
 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 = "$B227ドル"
 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 = "$B269ドル"
 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 = "$B306ドル"
 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 = "$B331ドル"
 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 = "$B373ドル"
 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 = "$B406ドル"
 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 = "$B425ドル"
 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 = "$B610ドル"
 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 = "$B5004ドル"
 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 = "$B5104ドル"
 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 = "$B5118ドル"
 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
asked Nov 26, 2019 at 14:04
\$\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

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 = "$B4ドル"
 EntryB4
 Case Is = "$B5ドル"
 EntryB5
 Case Is = "$B6ドル"
 EntryB6
 Case Is = "$B7ドル"
 EntryB7
 Case Is = "$B8ドル"
 EntryB8
 Case Is = "$B9ドル"
 EntryB9
 Case Is = "$B10ドル"
 EntryB10
 Case Is = "$B11ドル"
 EntryB11
 Rem Wires from Deposit Account or Loan (Post-Closing) Section
 Case Is = "$B205ドル"
 EntryB205
 Case Is = "$B227ドル"
 EntryB227
 Case Is = "$B269ドル"
 EntryB269
 Rem Loan-Closing Wires Section
 Case Is = "$B306ドル"
 EntryB306
 Case Is = "$B331ドル"
 EntryB331
 Case Is = "$B373ドル"
 EntryB373
 Rem Cash Management Wires Section
 Case Is = "$B406ドル"
 EntryB406
 Case Is = "$B425ドル"
 EntryB425
 Rem Internal Foresight Wires Section
 Case Is = "$B610ドル"
 EntryB610
 Rem Wire Transfer Agreement Section
 Case Is = "$B5004ドル"
 EntryB5004
 Rem Recurring Wire Transfer Request Section
 Case Is = "$B5104ドル"
 EntryB5104
 Case Is = "$B5118ドル"
 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
answered Nov 27, 2019 at 12:25
\$\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("$B3ドル")

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 $ABC678023983,ドル 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 = "$B4ドル": 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 = "$B4ドル"
 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!

answered Nov 26, 2019 at 20:16
\$\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

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.