5
\$\begingroup\$

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
\$\endgroup\$
  • 1
    \$\begingroup\$ Depends on what ChangeG and ChangeE subs do for you... \$\endgroup\$ – Chrismas007 Jan 16 '15 at 15:57
  • 1
    \$\begingroup\$ It would be nice to see those subs too, but we can certainly review what's here. \$\endgroup\$ – RubberDuck Jan 16 '15 at 15:58
  • \$\begingroup\$ @RubberDuck Application.Proper(Target.Value) = "Whatev" would clean up those redundancies right? \$\endgroup\$ – Chrismas007 Jan 16 '15 at 16:01
8
\$\begingroup\$
  • 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
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ One more thing to add to this excellent answer: it's important to remember that Target can be a multi-cell range (eg I can use Crtl-Enter to fill multiple cells in one operation). So it's best to code this type of event with that in mind. \$\endgroup\$ – Tim Williams Jan 16 '15 at 18:00
  • \$\begingroup\$ You're very right @TimWilliams. Good catch! \$\endgroup\$ – RubberDuck Jan 16 '15 at 18:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy