HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Copy Cell on Worksheet_Change event if some criteria is met

Submitted by: @import:stackexchange-codereview··
0
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 Sub

Solution

-
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 < 8


means the same thing as Target.Column = 7. Same goes for

Target.Column > 4 And Target.Column < 6


That 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 If


Note 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 Sub


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. 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 Sub

Code 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 Sub
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
Target.Column > 6 And Target.Column < 8
Target.Column > 4 And Target.Column < 6
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

Context

StackExchange Code Review Q#77729, answer score: 8

Revisions (0)

No revisions yet.