Excel VBA Hide Rows Based Upon List Selection Performance

Question

I currently have a worksheet with approximately 40K rows and 108 columns that I am working with. What I am doing is creating a list that when user selects a particular text in that list then certain columns and/or rows are hidden.

The issue I am encountering is that when 'Unsecured Streamline' is selected, the code to hide the rows takes a REALLY long time to run. Is there a more efficient way to execute what I am trying to achieve here?

Here is the portion of the code I am referring to:

ElseIf Target.Text = "UNSECURED STREAMLINE" Then
 Range("A:DD").EntireColumn.Hidden = False
 Range("AA:AM").EntireColumn.Hidden = True
 For Each cell In ActiveWorkbook.ActiveSheet.Columns("R").Cells
 If cell.Value <> "AP" Then
 cell.EntireRow.Hidden = True
 End If
 Next cell

Here is something else I have tried that works but still takes far too long:

For Each cell In Intersect(Me.UsedRange, Me.Columns("R"))
 If cell.Value <> "AP" Then
 cell.EntireRow.Hidden = True
 End If
Next cell

Here is the full code:

Private Sub Worksheet_Change(ByVal Target As Range)
 If Target.Address = ("$D1ドル") Then
 If Target.Text = "ALL" Then
 Range("A:DD").EntireColumn.Hidden = False
 ElseIf Target.Text = "OVERRIDES" Then
 Range("A:DD").EntireColumn.Hidden = False
 Range("A:C,J:O,AG:AQ,AX:AY,BA:BA,BC:BD,BF:BH,BJ:BK,BP:BQ,BT:BT,BW:BZ,CA:CB,CC:CI,CK:CK,CQ:CQ,CT:DD").EntireColumn.Hidden = True
 ElseIf Target.Text = "RATES" Then
 Range("A:DD").EntireColumn.Hidden = False
 Range("A:C,J:O,P:P,AB:AE,AG:AG,AR:AW,BE:BF,BW:CB,CE:CI,CK:CN,CR:CS,DB:DD").EntireColumn.Hidden = True
 ElseIf Target.Text = "UNSECURED STREAMLINE" Then
 Range("A:DD").EntireColumn.Hidden = False
 Range("AA:AM").EntireColumn.Hidden = True
 For Each cell In ActiveWorkbook.ActiveSheet.Columns("R").Cells
 If cell.Value <> "AP" Then
 cell.EntireRow.Hidden = True
 End If
 Next cell
 End If
 End If
End Sub

I think one issue may be that I am hiding each row individually but I do not know how to solve for this. Any assistance would be greatly appreciated since, as you can probably tell, I am quite new to VBA.

• Mathieu Guindon: Consider running your code through Rubberduck code inspections and indenter for a first-pass at a code review that eliminates the low-hanging fruits. As for your specific performance issue, search this site for VBA posts containing "union" for a neat little trick - this one seems relevant, but (and now I'm wondering if this comment should be an answer) you'll also want to probably only iterate the cells with data rather than the entire billion-row full column range.

Answer 1

Even though the handler doesn't modify any cells, it's usually a good idea to disable application events when handling a worksheet's Change event. You can do this by setting Application.EnableEvents to False at the top, and back to True before exiting - preferably with proper error handling, so the initial state is always restored whether there's a run-time error or not.

I mentioned Rubberduck in a comment earlier; its code inspections would have warned you about implicit qualifiers everywhere you're invoking the worksheet's Range property: Range(...) is implicitly Me.Range(...) here, meaning we're looking for cells on the same sheet as Target, i.e. the sheet that's handling this Change event.

However... not all worksheet accesses are implicitly qualified: ActiveWorkbook.ActiveSheet.Columns("R").Cells refers specifically to the active sheet, which may or may not be the same sheet. If there's code anywhere that can change cell $D1ドル while another sheet is active, then that's a bug, because you'll be iterating cells and hiding rows on a worksheet you're not intending to modify here.

You'll want to restrict the number of cells you're looking at, and avoid iterating every single cell in any given column - I suspect that's where most of the performance is getting lost, churning through hundreds of thousands of empty rows.

Consider first finding the last row you're interested in, and you can likely leave out the parameterless .Cells call:

For Each cell In Me.Range("R" & lastRow)
 ...
Next

You're reading cell values and comparing them to string literals without first validating that you can actually do that:

If cell.Value <> "AP" Then

This is dangerous: you'll get a type mismatch run-time error if a cell ever contains a worksheet error (like #N/A, or #VALUE!). Use IsError| to validate cell values first - because If` conditions don't short-circuit in VBA, that means nesting the conditions:

If Not IsError(cell.Value) Then
 If cell.Value <> "AP" Then
 ...
 End If
End If

But why are we using Value here but Text everywhere else? Pick one, and stick to it!

Now, if that ... inside the conditional block remains "hide that entire row", there's a missed opportunity to hide all the interesting rows at once! Use Union to build yourself a range as you traverse the cells, as if you were holding the Ctrl key to select rows; then you hide all the rows of the combined range, and benefit from only accessing the worksheet once.

Lastly, since all the conditions are checking different possible values for Target.Text, consider using Select Case Target.Text instead, to reduce redundancy and only read the value once:

Select Case Target.Text
 Case "ALL"
 '...
 Case "OVERRIDES"
 '...
 ...
End Select
```

• Brandon: I appreciate this. I am working through it trying to understand things further. For Each cell In Me.Range("R" & lastRow)
• Mathieu Guindon: @Brandon see this Stack Overflow answer for how to do that. Hope it helps!
• Brandon: I appreciate this. I am working through it trying to understand things further. When I use the piece of code For Each cell In Me.Range("R" & lastRow) I get "Method 'Range' of object _Worksheet not found' message. I have not been able to understand how to fix that. Also, the idea of the union is wonderful. Is that something in which I would need to create two dimensions for at the top? One for the rows to be hidden and then one for the ones not to be hidden. Then use the union, and then hide the rows correct?
• Brandon: am I on the right track in regards to using the union correctly? Setting this up top Dim AP As Range, NotAP As Range and then down below For Each cell In Me.Range("R3", .Range("R" & .Rows.Count).End(xlUp))' and then below that If cell.Text <> "AP" Then Set AP = NotAP Else Set AP = Union(AP, NotAP)`.