Hiding and showing spreadsheet rows based on cell value I'm currently using the following code that is triggered as each cell in a column is updated.  It works fine, but takes a couple seconds for every cell, which is really cumbersome with ~200 cells to complete the sheet.  Is there any way to speed up this code? 

Private Sub Worksheet_Change(ByVal Target As Range)

Application.ScreenUpdating = False
Sheet1.Cells.EntireRow.AutoFit


Dim iLastRow As Variant
Dim i As Integer
iLastRow = Range("R1:R" &amp; Cells(Rows.Count, "R").End(xlUp).Row).Value

For i = 1 To UBound(iLastRow, 1)
    If Range("R" &amp; i) = "Show" Then
        Rows(i).EntireRow.Hidden = False
    Else
        Rows(i).EntireRow.Hidden = True
    End If
Next

Application.ScreenUpdating = True

End Sub
You have indented the blocks, but not the scopes.

Private Sub DoSomething()
    '&lt;~ here
End Sub


Might not be the case here, but in a module with multiple procedures, it's much easier to glance at the code and see where scopes begin &amp; end when their contents are indented.




Application.ScreenUpdating = False



That's great: Excel isn't going to be wasting cycles repainting itself everytime you make a change. There's a problem though:


Application.ScreenUpdating = True



If there's a runtime error anywhere between these two instructions, execution jumps out of the procedure scope and ScreenUpdating never gets set back to True, and because the error is never handled, well, you have a potentially unhandled runtime error waiting to be thrown without warning in your user's face.

Right here:


If Range("R" &amp; i) = "Show" Then



If Range("R" &amp; i) contains an error value, VBA can't compare it to a String literal, and boom, runtime error 13, type mismatch. You should always assume that a worksheet cell can contain an error (#VALUE!, #NA, #REF!, etc.), and shield your code against that classic mistake using the IsError function:

If Not IsError(Range("R" &amp; i)) Then


In any case, whenever you toggle Application state, the procedure should have an On Error statement that ensures state is cleanly toggled back in case of an error.

Private Sub DoSomething()
    On Error GoTo CleanFail
    Application.ScreenUpdating = False

    '...do stuff...

CleanExit:
    Application.ScreenUpdating = True
    Exit Sub
CleanFail:
    '...handle error...
    Resume CleanExit
End Sub





Dim iLastRow As Variant



This means to be a row number, and given the Systems Hungarian prefix (drop it!), it has all looks of an Integer. It should be a Long, because a 16-bit signed integer couldn't work with more than 32,767 rows - and a worksheet can have many, many more rows than that. Internally, the VBA7 runtime allocates a 32-bit integer for it anyway, so you might as well use a Long.

Except you can't.


iLastRow = Range("R1:R" &amp; Cells(Rows.Count, "R").End(xlUp).Row).Value



Because it's actually an array, that contains every single value in column R!

That's actually not a bad idea in itself, except the only thing you're using it for, is to determine how many "cells" it contains - here:


For i = 1 To UBound(iLastRow, 1)



Write code that says what it means, and means what it says. If you name a variable "last row", make it hold the last row number - not a variant array that contains every single value in a column. Or, name it visibilityToggleColumn or something similar.

i being an Integer is also problematic. Use a Long, so you don't need to worry about how many rows there might be - there's no reason to use an Integer for this.



This Boolean assignment is a common pattern:


If Range("R" &amp; i) = "Show" Then
    Rows(i).EntireRow.Hidden = False
Else
    Rows(i).EntireRow.Hidden = True
End If



So common, the latest 2.1 pre-release of Rubberduck includes an inspection (and a quick-fix) for it:


  Boolean literal assignment in conditional
  
  A member is assigned True/False in different branches of an If statement with no other statements in the conditional. Use the condition directly to assign the member instead.


Applying the quickfix turns this:


    If Range("R" &amp; i) = "Show" Then
        Rows(i).EntireRow.Hidden = False
    Else
        Rows(i).EntireRow.Hidden = True
    End If



Into this:


    Rows(i).EntireRow.Hidden = Not (Range("R" &amp; i) = "Show")



Which can then be simplified to this:


    Rows(i).EntireRow.Hidden = Range("R" &amp; i) &lt;&gt; "Show"





Rubberduck also complains about implicit references to ActiveSheet - these are the root cause behind roughly 40%1 of all VBA questions asked on Stack Overflow, every day.

Unqualified Range, Rows, Columns, Names, and Cells calls, all implicitly refer to the ActiveSheet... when you're in a standard module. When you're in a worksheet module's code-behind, they implicitly refer to that sheet. The simple fact that the same identical code does different things depending on where it's written, should be enough to be explicit about it.

Since this is all happening behind Sheet1, IMO the ideal qualifier would be Me. So instead of this:


Sheet1.Cells.EntireRow.AutoFit



I would have that:


Me.Cells.EntireRow.AutoFit



And then here:


iLastRow = Range("R1:R" &amp; Cells(Rows.Count, "R").End(xlUp).Row).Value



I'd have this:


iLastRow = Me.Range("R1:R" &amp; Me.Cells(Me.Rows.Count, "R").End(xlUp).Row).Value



Note that if you're in the code-behind for Sheet1, you can interchangeably use Sheet1 and Me, ...but you shouldn't. Consistency is king. Seeing Me as a qualifier immediately reminds the reader that they're looking at code that's in a class module (a Worksheet is a class, Sheet1 is a Worksheet instance; ditto for Workbook and ThisWorkbook). On the other hand, qualifying with a specific worksheet's codename makes that code usable from anywhere, i.e. if you want to move that code from Sheet1.Worksheet_Change to the ThisWorkbook.SheetChange handler, nothing would have to change and it would work identically.



Okay got it. Now what about performance?

Worksheet manipulations are inherently slow. Reading &amp; writing cell values is slow, and resizing and formatting cells is even slower.

Surely you know of the "hold Ctrl down" trick? Hold down the Ctrl key, click on the row headings you want to hide, right-click once, pick "hide" once, and poof all the selected rows get hidden at once!

You can do exactly the same thing in VBA, using a little Union helper function - something like this:

Private Function CombineRanges(ByVal source As Range, ByVal toCombine As Range) As Range
    If source Is Nothing Then
        Set CombineRanges = toCombine
    Else
        Set CombineRanges = Union(source, toCombine)
    End If
End Function


Now all you'll be doing with Range objects, is dereferencing them - as opposed to accessing their members and mutating their state.

Before I share the resulting code, I'd like to add that there are probably certain things you want to abstract away into other members. I'd declare module-level Const values, to avoid magic literals, and I'd probably expose a Property Get members or two, to abstract as many concepts and expose them to other code if they need to - otherwise make them Private, doesn't hurt.

So anyway, here's my version:

Option Explicit
Private Const VisibilityToggleColumn As Long = 18 'column "R"
Private Const VisibilityToggleValue As String = "Show"

Public Property Get LastRow() As Long
    LastRow = Me.Cells(Me.Rows.Count, VisibilityToggleColumn).End(xlUp).Row
End Property

Public Property Get VisibilityToggleColumnValues() As Variant
    VisibilityToggleColumnValues = Me.Range(Me.Cells(1, VisibilityToggleColumn), Me.Cells(LastRow, VisibilityToggleColumn)).Value
End Property

Private Sub Worksheet_Change(ByVal Target As Range)

    On Error GoTo CleanFail
    ToggleApplicationState False

    Me.Cells.EntireRow.AutoFit

    Dim rowsToHide As Range
    Dim rowsToShow As Range

    Dim visibilityToggleValues As Variant
    visibilityToggleValues = VisibilityToggleColumnValues

    Dim i As Long
    For i = LBound(visibilityToggleValues) To UBound(visibilityToggleValues)

        Dim currentCell As Range
        Set currentCell = Me.Cells(i, VisibilityToggleColumn)

        Dim currentToggle As Variant
        currentToggle = visibilityToggleValues(i, 1)

        If IsError(currentToggle) Then
            'if a cell contains an error, we don't want to hide it
            Set rowsToShow = CombineRanges(rowsToShow, currentCell)
        Else
            If StrComp(currentToggle, VisibilityToggleValue, vbTextCompare) = 0 Then
                Set rowsToShow = CombineRanges(rowsToShow, currentCell)
            Else
                Set rowsToHide = CombineRanges(rowsToHide, currentCell)
            End If
        End If

    Next

    If Not rowsToShow Is Nothing Then rowsToShow.EntireRow.Hidden = False
    If Not rowsToHide Is Nothing Then rowsToHide.EntireRow.Hidden = True

CleanExit:
    ToggleApplicationState True
    Exit Sub
CleanFail:
    MsgBox "Unhandled error in 'Worksheet_Change' handler: " &amp; Err.Description, vbExclamation
    Resume CleanExit
    Resume
End Sub

Private Sub ToggleApplicationState(ByVal enabled As Boolean)
    Application.ScreenUpdating = enabled
    Application.EnableEvents = enabled
    Application.Calculation = IIf(enabled, xlCalculationAutomatic, xlCalculationManual)
End Sub

Private Function CombineRanges(ByVal source As Range, ByVal toCombine As Range) As Range
    If source Is Nothing Then
        Set CombineRanges = toCombine
    Else
        Set CombineRanges = Union(source, toCombine)
    End If
End Function


Notice I'm using the StrComp function to compare the strings with vbTextCompare - this ensures a case-insensitive comparison without the extra processing overhead of LCase/LCase$ or UCase/UCase$ conversions.

That code runs in just over 200ms2, with over 13,000 rows in column R - and there's probably still more room for improvements, but I'll leave it at that for now.



1By no scientific measurement whatsoever. It's just an extremely common mistake.

2Lazily measured with a very basic Timer diff.