Slow Excel VBA Loop - Looking for improvement suggestions

Question

I created a VBA macro in Excel linked to a button that hides or unhides specific rows.

I want the button to be able to hide/unhide rows 5 and 6, 10 and 11, 15 and 16, 20, 21, 25, 26, 30, 31, etc... up to row 400 roughly.

Below are two macros that I've used to achieve this, both work, though the top one is slightly faster than the bottom one.

Can anyone suggestions ways to improve this further?

Private Sub HidePlannedNEW_Click()

    Dim i As Long

    For i = 5 To 50 Step 4

        If Rows(i).Hidden = False Then
            Rows(i).Hidden = True
            i = i + 1
            Rows(i).Hidden = True

        ElseIf Rows(i).Hidden = True Then
            Rows(i).Hidden = False
            i = i + 1
            Rows(i).Hidden = False

        End If

    Next i

End Sub

(for this second macro to work I created a helper column with the letter 'P' in the rows I wanted to hide)

Private Sub HidePlannedOLD_Click()

    For Each i In ActiveSheet.Range("D4:D600")

        If i.EntireRow.Hidden = False And i.Value = "P" Then
             i.EntireRow.Hidden = True

        ElseIf i.EntireRow.Hidden = True And i.Value = "P" Then
             i.EntireRow.Hidden = False

        End If

    Next i

End Sub

Many thanks in advance!

I'm fairly new to VBA in general so any comments/suggestions welcome. — NathanG, Jan 07 '20 at 10:58
I'm voting to close this question as off-topic because If you are just looking for improvements then your question belongs in code review instead. https://codereview.stackexchange.com/ — braX, Jan 07 '20 at 10:58

score 0 · Accepted Answer · answered Jan 07 '20 at 11:14

First a general remark:

In your first macro, there's one thing that frightens me: you modify the counter variable, which might lead to unforeseen results. Instead of:

For i = 5 To 50 Step 4

    If Rows(i).Hidden = False Then
        Rows(i).Hidden = True // at the first step, i = 5
        i = i + 1
        Rows(i).Hidden = True // now i = 6, so step 4 will cause i = 10
                              // this is indeed what you want, but it makes your code
                              // unreadable (imagine you look at this again within a year,
                              // will you still understand what you have written here?)

Do the following:

For i = 5 To 50 Step 5

    If Rows(i).Hidden = False Then
        Rows(i).Hidden = True
        Rows(i + 1).Hidden = True

Now, as far as your question is concerned: I believe that you are hiding one row after the other, but in this post there's an explanation of how to combine cells to one range. You might combine cells to one range, take the entire rows of that range, and only perform one .Hidden = True command, this will reduce the amount of screen updates and improve the speed of your macro.

Thank you very much for the responce, very useful! Screen updates, as you menitoned briefly at the end of your post, I believe are the culprit for this macro seeming slow as it currently either hides each row individually (old macro) or two rows at a time (new macro). I'm now exploring ways to identify all the rows that need to be hidden, feeding that into a range, then hiding all the required rows in a single hit. — NathanG, Jan 07 '20 at 16:31

Slow Excel VBA Loop - Looking for improvement suggestions

1 Answers1