You have indented the blocks, but not the scopes.
Private Sub DoSomething()
'<~ 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 & 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" & i) = "Show" Then
If Range("R" & 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" & 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" & 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" & 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" & 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" & i) = "Show")
Which can then be simplified to this:
Rows(i).EntireRow.Hidden = Range("R" & i) <> "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" & Cells(Rows.Count, "R").End(xlUp).Row).Value
I'd have this:
iLastRow = Me.Range("R1:R" & 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 & 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: " & 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.
