2
\$\begingroup\$

I would like to ask for a code review as I feel like it's not the most efficient way of doing it. For context I have a add-ins with all the code in module and then the SheetChangeevent is in ThisWorkbook. Also in the add-ins I have 2 sheets that will have data on it so that the ActiveWorkbook will be able to read this info when running code form module the events will fire and do a vlookup against the sheet in the add-ins.

Private Sub ExcelApp_SheetChange(ByVal Sh As Object, ByVal Target As Range)
    Dim KeyCells As Range
    Dim wsMyList As Worksheet
    Set wsMyList = ThisWorkbook.Sheets(2)

    Set KeyCells = [B3,B5] 'I only need this 2 cells to fire events
    If Sh.Name <> "Response" Then
         If Not Intersect(Target, Sh.Range("B3:B5")) Is Nothing Then 'not too sure how to do it here so I put 3 cells instead of 2
            If Target.Row = 3 Then
                If Range("B3").Value = vbNullString Then Exit Sub
                Application.EnableEvents = False
                If Sh.Range("B3").Value <> vbNullString Then
                    Sh.Range("B4").Value = Application.WorksheetFunction.VLookup(Sh.Range("B3").Value, wsMyList.Range("A:B"), 2, False)
                    Sh.Range("B6").Value = "Type or Select a transaction"
                Else
                    Sh.Range("B4").Value = "Type or Select a program"
                End If
                Columns.ClearColumns
                Transactions.FetchTransactions
                Application.EnableEvents = True
            ElseIf Target.Row = 5 Then
                If Range("B5").Value = vbNullString Then Exit Sub
                Application.EnableEvents = False
                If Range("B5").Value <> vbNullString Then
                    Sh.Range("B6").Value = Application.WorksheetFunction.VLookup(Sh.Range("B5").Value, wsMyList.Range("D:E"), 2, False)
                    Columns.ClearColumns
                    Columns.PopulateFields
                End If
                Application.EnableEvents = True
            End If
        End If
    End If
End Sub
\$\endgroup\$
5
\$\begingroup\$

When writing any kind of event method, I always try to minimize the code that executes because you don't want the user to be aware of processing that is happening between the keystrokes. In your case, your code is firing each time ANY cell on ANY sheet is changed. So in the spirit of keeping things streamlined, don't create, initialize, or perform any logic that you don't really need (until you need it). Using this philosophy, the beginning of my example method would look like this:

If Sh.Name = "Response" Then Exit Sub

Dim checkCells As Range
Set checkCells = Union(Sh.Range("B3"), Sh.Range("B5"))
If Intersect(Target, checkCells) Is Nothing Then Exit Sub
If Target.Address = vbNullString Then Exit Sub

Notice that if the changed sheet is "Response", then initializing any other variable is meaningless. Once we get past that, a checkCells range is established using the Union function. It may be a bit overkill sometimes, but clearly illustrates the idea that you're looking at multiple, non-contiguous cells/ranges.

Also, from your OP code, if either cell is empty, you are immediately exiting. So you can check the target address after the other checks right away.

But then I get into an issue where you check if the cell value is null, but right away you're checking that it has a value:

If Range("B3").Value = vbNullString Then Exit Sub
Application.EnableEvents = False
If Sh.Range("B3").Value <> vbNullString Then ...

This is redundant. And -- by the way -- you'll never get to your Else statement because you've already exited if B3 is a null string.

The statement in both parts of your If statement is confusing:

Columns.ClearColumns

Columns is a property of a Range or Worksheet, and ClearColumns is not a method of Range at all that I know of. So I'm assuming it's a part of the add-in. But if Columns is the name of one of your code modules, then change it. Using Columns is not a good name to use because it is the same as an existing property and is confusing. If you are clearing columns on a worksheet, then specify which worksheet always. I'm also assuming the Transactions is a code module in your VBA project as well.

Here is all of the code:

Option Explicit

Private Sub Workbook_SheetChange(ByVal Sh As Object, ByVal Target As Range)
    If Sh.Name = "Response" Then Exit Sub

    Dim checkCells As Range
    Set checkCells = Union(Sh.Range("B3"), Sh.Range("B5"))
    If Intersect(Target, checkCells) Is Nothing Then Exit Sub
    If Target.Address = vbNullString Then Exit Sub

    Dim wsMyList As Worksheet
    Set wsMyList = ThisWorkbook.Sheets(2)

    Application.EnableEvents = False
    With Sh
        Dim lookupArea As Range
        If Target.Row = 3 Then
            Set lookupArea = wsMyList.Range("A:B")
            .Range("B4").Value = Application.WorksheetFunction.VLookup(Target.Value, _
                                                                       lookupArea, _
                                                                       2, False)
            .Range("B6").Value = "Type or Select a transaction"
            Columns.ClearColumns
            Transactions.FetchTransactions
        ElseIf Target.Row = 5 Then
            Set lookupArea = wsMyList.Range("D:E")
            .Range("B6").Value = Application.WorksheetFunction.VLookup(Target.Value, _
                                                                       lookupArea, _
                                                                       2, False)
            Columns.ClearColumns
            Columns.PopulateFields
        End If
    End With
    Application.EnableEvents = True
End Sub
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ thanks for the feedback and all your assumptions are correct. I will modify everything accordingly. Also if you have time are you able to provide any feedback on the vlookup because I ended up putting on error resume next and on error goto 0 before and after that line as it was failing in some conditions. Thanks a lot. \$\endgroup\$ – QuickSilver Mar 27 at 16:12
  • \$\begingroup\$ I would suggest you change your VLookup to use Application.VLookup instead. See this guide for more information. \$\endgroup\$ – PeterT Mar 27 at 18:57

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.