patternMinor
Copy Cell on Worksheet_Change event if some criteria is met
Viewed 0 times
cellworksheet_changemetcriteriacopysomeevent
Problem
This code is working fine, but it looks awful (yes, I am a rookie on this thing). How can I improve it to look better?
Private Sub Worksheet_Change(ByVal Target As Range)
'Conditional Formatting
Application.ScreenUpdating = False
If Target.Column > 6 And Target.Column 4 And Target.Column < 6 And (Target.Value = "High" Or Target.Value = "Medium" Or Target.Value = "Low" Or Target.Value = "high" Or Target.Value = "medium" Or Target.Value = "low") Then
Sheets("Pending").Cells(1, 1).Copy
Call ChangeE
End If
Application.ScreenUpdating = True
End SubSolution
-
Indent everything one level inside of the
-
There shouldn't be more than one blank line between instructions.
-
This condition can be simplified.
means the same thing as
That can simply be
-
I don't normally advise using line continuations, but they can be justifiably used to reduce the amount of horizontal scrolling.
-
That last change brought to light the fact that you want to match on upper or lower case versions of these letters. Using VBA's Like operator will help clean that up with some built in pattern matching.
Note that it cleans it up so well, that the line continuation isn't needed anymore.
-
Don't use the
-
The second condition is a little harder to clean up, but we can still use
-
You've hard coded the string literal for the
Note that I used the
And there ya go. All cleaned up.
One last thing that I have not addressed, but was mentioned in the comments by Tim Williams. There is a potential bug waiting to rear its ugly little head.
Indent everything one level inside of the
Sub...End Sub block. Go one more inside of If blocks and loops.Private Sub Worksheet_Change(ByVal Target As Range)
' all code starts at this level of indentation
If condition Then
' one more level
End If
End Sub-
There shouldn't be more than one blank line between instructions.
Private Sub Worksheet_Change(ByVal Target As Range)
'Conditional Formatting
Application.ScreenUpdating = False
If Target.Column > 6 And Target.Column < 8 And (Target.Value = "G" Or Target.Value = "Y" Or Target.Value = "R" Or Target.Value = "g" Or Target.Value = "y" Or Target.Value = "r") Then
Sheets("Pending").Cells(1, 1).Copy-
This condition can be simplified.
Target.Column > 6 And Target.Column < 8means the same thing as
Target.Column = 7. Same goes for Target.Column > 4 And Target.Column < 6That can simply be
Target.Column = 5.-
I don't normally advise using line continuations, but they can be justifiably used to reduce the amount of horizontal scrolling.
If Target.Column = 7 And _
(Target.Value = "G" _
Or Target.Value = "Y" _
Or Target.Value = "R" _
Or Target.Value = "g" _
Or Target.Value = "y" _
Or Target.Value = "r") Then
Sheets("Pending").Cells(1, 1).Copy
Call ChangeG
End If-
That last change brought to light the fact that you want to match on upper or lower case versions of these letters. Using VBA's Like operator will help clean that up with some built in pattern matching.
If Target.Column = 7 And Target.Value Like "[GgYyRr]" Then
Sheets("Pending").Cells(1, 1).Copy
Call ChangeG
End IfNote that it cleans it up so well, that the line continuation isn't needed anymore.
-
Don't use the
Call keyword. It's unneeded and archaic. It's simply a leftover from an ancient version of VBA. -
The second condition is a little harder to clean up, but we can still use
Like to make it better.If Target.Column = 5 And _
(Target.Value Like "[Hh]igh" _
Or Target.Value Like "[Mm]edium" _
Or Target.Value Like "[Ll]ow") Then
Sheets("Pending").Cells(1, 1).Copy
ChangeE
End If-
You've hard coded the string literal for the
"Pending" sheet twice. It would be better to initialize a Worksheet variable upon entering the event routine.Dim sourceSheet As Worksheet
Set sourceSheet = Worksheets("Pending")Note that I used the
Worksheets collection instead of Sheets. That's because the Sheets collection can contain charts as well as worksheets. It's a rare issue, but it can be one.And there ya go. All cleaned up.
Private Sub Worksheet_Change(ByVal Target As Range)
Application.ScreenUpdating = False
Dim sourceSheet As Worksheet
Set sourceSheet = Worksheets("Pending")
If Target.Column = 7 And Target.Value Like "[GgYyRr]" Then
sourceSheet.Cells(1, 1).Copy
ChangeG
End If
If Target.Column = 5 And _
(Target.Value Like "[Hh]igh" _
Or Target.Value Like "[Mm]edium" _
Or Target.Value Like "[Ll]ow") Then
sourceSheet.Cells(1, 1).Copy
ChangeE
End If
Application.ScreenUpdating = True
End SubOne last thing that I have not addressed, but was mentioned in the comments by Tim Williams. There is a potential bug waiting to rear its ugly little head.
Target can be a multi-cell range object and Range.Value is not available in multi-cell ranges. You need to be prepared to deal with that situation should it arise. Consider the example below.Public Sub test()
Dim Workbook As Workbook
Set Workbook = ThisWorkbook
Dim sheet As Worksheet
'Set sheet = Workbook.Worksheets("Sheet1")
Set sheet = Workbook.Worksheets(1)
Dim rng1 As Range
Set rng1 = sheet.Range("C2")
Dim rng2 As Range
Set rng2 = sheet.Range("C2:C10")
Debug.Print rng1.Value ' prints the value stored in C2
Debug.Print rng2.Value ' raises type mismatch error #13
End SubCode Snippets
Private Sub Worksheet_Change(ByVal Target As Range)
' all code starts at this level of indentation
If condition Then
' one more level
End If
End SubPrivate Sub Worksheet_Change(ByVal Target As Range)
'Conditional Formatting
Application.ScreenUpdating = False
If Target.Column > 6 And Target.Column < 8 And (Target.Value = "G" Or Target.Value = "Y" Or Target.Value = "R" Or Target.Value = "g" Or Target.Value = "y" Or Target.Value = "r") Then
Sheets("Pending").Cells(1, 1).CopyTarget.Column > 6 And Target.Column < 8Target.Column > 4 And Target.Column < 6If Target.Column = 7 And _
(Target.Value = "G" _
Or Target.Value = "Y" _
Or Target.Value = "R" _
Or Target.Value = "g" _
Or Target.Value = "y" _
Or Target.Value = "r") Then
Sheets("Pending").Cells(1, 1).Copy
Call ChangeG
End IfContext
StackExchange Code Review Q#77729, answer score: 8
Revisions (0)
No revisions yet.