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
2 Answers 2
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
-
\$\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\$Zack E– Zack E2019年11月27日 12:32:04 +00:00Commented Nov 27, 2019 at 12:32
-
\$\begingroup\$ @ZackE Thanks for accepting my answer! \$\endgroup\$TinMan– TinMan2019年11月27日 16:29:01 +00:00Commented 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\$Zack E– Zack E2019年11月27日 16:32:41 +00:00Commented Nov 27, 2019 at 16:32
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!
-
\$\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\$Zack E– Zack E2019年11月26日 20:56:49 +00:00Commented 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\$Zack E– Zack E2019年11月27日 14:04:56 +00:00Commented Nov 27, 2019 at 14:04
-
\$\begingroup\$ no problems, glad you found them both useful. \$\endgroup\$AJD– AJD2019年11月27日 18:41:14 +00:00Commented Nov 27, 2019 at 18:41
B5
? You have two switches with the same criteria, one nested in the other. Looks like you could pull the secondselect 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 aFunction()
, i don't believe it would be appropriate to use whenTarget
is literally the cell and adding the function would be redundant. \$\endgroup\$Function()
I can see what you are saying. \$\endgroup\$