# SheetChange event VBA

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

• Please could you flesh out the description, currently it conveys nothing to me. – Peilonrayz Apr 6 '20 at 20:35
• @Peilonrayz sorry what do you mean? I don’t understand your ask. – QuickSilver Apr 7 '20 at 7:32
• 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? – Reinderien Apr 13 '20 at 17:08

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

• 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. – QuickSilver Mar 27 '20 at 16:12
• I would suggest you change your VLookup to use Application.VLookup instead. See this guide for more information. – PeterT Mar 27 '20 at 18:57