Copy Cell on Worksheet_Change event if some criteria is met

Question

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

Call ChangeG
End If

If 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

Answer 1

• 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