SheetChange event VBA

Question

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

• Peilonrayz: Please could you flesh out the description, currently it conveys nothing to me.
• QuickSilver: @Peilonrayz sorry what do you mean? I don’t understand your ask.
• Reinderien: Please attempt to provide a more general description of the code's purpose. What are you doing with Excel and why? What are your specific efficiency concerns?

Answer 1

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

• QuickSilver: 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.
• PeterT: I would suggest you change your VLookup to use Application.VLookup instead. See this guide for more information.