2
\$\begingroup\$

I have process where I am generating workbooks from a list. One of the tabs has varying height and I have a formula that my macro uses to determine whether to hide a row.

As I move to the next workbook I may need to unhide some rows that were hidden in the previous iteration. For the sake of better understanding excel and getting better performance from my macros I'm wondering if it's faster to utilize the Else condition to unhide the rows as it iterates, since it will already be doing so or if it's faster to simply unhide all the rows again in a separate macro and exclude an else condition.

Basically, curious which is the more optimal process. At first, I thought using another macro to skip all the iterating and logic and simply unhide all the rows before hiding them as needed again would be faster, but now thinking about it since it's already iterating through I'm curious it is actually slower.

Sub DoProcess()

Call UnhideRows
Call HideRows

End Sub
Sub UnHideRows()

Call DefineVariables
   
    Sheet1.Rows("13:37").Rows.EntireRow.Hidden = False

End Sub
Sub HideRows()

Call DefineVariables
For x = 13 to 37
    If Sheet1.Cells(x,7).Value = "Hide"
        Sheet1.Cells(x,7).EntireRow.Hidden = True
    End If
Next
End Sub

Or is it better to accomplish this with this approach:

Sub HideUnhideRows()

Call DefineVariables

For x = 13 To 37
    If Sheet1.Cells(x, 7).Value = "Hide" Then
        Sheet1.Cells(x, 7).EntireRow.Hidden = True
    Else
        Sheet1.Cells(x, 7).EntireRow.Hidden = False
    End If
Next

End Sub
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Worth a read: Which is Faster? \$\endgroup\$
    – ggorlen
    Apr 17 at 14:15
  • \$\begingroup\$ @ggorlen Fair link, as someone who's self-taught with this stuff is there a process that is considered cleaner, barring speed. The link definitely points out a few things such as this not being the bottleneck, but I imagine the second is generally just cleaner with less things macros to maintain and call. This is more a simplified example of what I'm working with, so hoped to get a little smarter with things by cleaning things up one at a time. \$\endgroup\$
    – Mark S.
    Apr 17 at 14:21
  • \$\begingroup\$ @MarkS. Code Review works well when you post real world code. You can include an extract and you will probably get a detailed breakdown, or an entire application and you will get a higher level review with the benefit of a lot more context. You don't want to leave too much open to interpretation because it may end up telling you things you already knew but just left out or removed while trying to make a summary. \$\endgroup\$
    – Greedo
    Apr 17 at 14:41
  • \$\begingroup\$ @Greedo thank you for the note, I'm still getting my footing on best practices here, usually from StackOverflow. So apologies if I'm still green in the best approach here to optimize. The concept is essentially this, but I do it on a few separate sheets across different length ranges, some static in height, others dynamic in height. But I 100% see what you're saying. \$\endgroup\$
    – Mark S.
    Apr 17 at 14:50

1 Answer 1

4
\$\begingroup\$

The answer is neither ;)

What you want is something along the lines of

Sub HideUnhideRows()

Call DefineVariables

Sheet1.Rows("13:37").EntireRow.Hidden = False
Dim x As Long
Dim stuffToHide As Range
For x = 13 To 37
    If Sheet1.Cells(x, 7).Value = "Hide" Then
        If stuffToHide is Nothing Then
            Set stuffToHide = Sheet1.Cells(x, 7).EntireRow
        Else
            Set stuffToHide = Union(stuffToHide, Sheet1.Cells(x, 7)8.EntireRow)
        End If
    End If
Next
stuffToHide.Hidden = True

End Sub

Basically unhiding everything like your first example, then building up the range piece by piece with Union and hiding it all at once.

At first, I thought using another macro to skip all the iterating and logic and simply unhide all the rows before hiding them as needed again would be faster, but now thinking about it since it's already iterating through I'm curious it is actually slower.

Yes your intuition is correct, either way the performance is O(n) where n is the number of rows in 13 -> 17, double the rows, half the speed (more or less).

The difference is whether you are doing the looping in VBA or getting Excel to do it for you. By hiding and unhiding entire blocks of cells rather than individual rows, the slow work is all happening in Excel.exe's compiled interior and is therefore very fast, rather than VBA. The real killer here is the interop - sending the unhide command to Excel and waiting for its response (including a screen repaint), that takes time. The actual overhead of iterating over cells or Excel marking the range as hidden will be tiny.

In effect the time taken for Excel to hide 1000 rows will be very similar to 1 row, since the majority of the time will be spent warming up (switching context from VBA to Excel) and doing the screen rendering, possibly recalculating volatile formulas etc. rather than performing the unhide operation (which is probably as simple as setting a 1 bit flag for each row affected - i.e. instantaneous).

All that is to say, by moving the looping out of VBA and into Excel, it basically becomes an O(1) constant time operation rather than O(n), double the rows, same amount of time. The reason you may find your app slow is because you are processing lots of rows. If the operation is O(1) then it doesn't matter how many rows you have.

Note we've added looping over the ranges and Union-ing them, which is still 0(n), but it's way faster per step of the loop as it doesn't trigger much interop, screen painting, or recalculating.


While we're here

Sub DoProcess()

Call UnhideRows
Call HideRows

End Sub

Call is Obsolete you don't need it anymore. It can look okay with arguments Call UnhideRows(rangeToUnhide) vs UnhideRows rangeToUnhide, but for a plain Sub call, just write UnhideRows without the Call since it is just noise that doesn't contribute to the meaningfulness of the code.

DoProcess is a bad ambiguous name. If this is the Entry point triggered by some button press or event, I like to name it OnXXX e.g. OnFinanceSummaryCreate

Also this method is too short and I would just put both in the body of this one rather than having those extra Subs

Call DefineVariables

This looks like it could be setting global variables, which are best avoided. Consider declaring some variables Dim x As Range instead.

You also call it twice in the first example and only once in the second, highlighting the risk with global variables.

Sheet1.Rows("13:37").EntireRow.Hidden

Good use of Sheet1 to qualify rather than Sheets("Sheet1"), it makes code much less brittle to the tabs being renamed. Did you know you can rename the "code name" to something more meaningful e.g. DashboardSheet in the properties page.

Also consider using a named range instead like DashboardSheet.Range("OutputRangeName").EntireRow. It makes it more resilient to adding and removing rows or shifting the template, since the named range will shift with them.

\$\endgroup\$
7
  • \$\begingroup\$ This was very thorough, thank you. As for notes regarding names like DoProcess, this is just for the question, not my real naming convention, just for the sake of articulating another macro that runs them in a particular order. \$\endgroup\$
    – Mark S.
    Apr 17 at 14:47
  • 1
    \$\begingroup\$ @MarkS. The code in the question must be your real code. \$\endgroup\$
    – pacmaninbw
    Apr 17 at 14:54
  • \$\begingroup\$ @pacmaninbw Noted, the difference is just the Sub name, variable name (sheet1). I left out things that wouldn't impact this component for the sake of isolating it so I can understand the problem in a vacuum. Apologies if I misunderstood here. \$\endgroup\$
    – Mark S.
    Apr 17 at 14:57
  • 1
    \$\begingroup\$ Apologies on spamming comments as I chip away at your notes and apply them and try to learn them. Cool you can change the sheet name in the properties, I haven't utilized that all. Regarding the named ranges that is what I use my global variables for. I have a sheet where I define a number of conditions like the date the report is for, iterating through our departments I'm running the report for to steer the formulas in the sheets and pivot tables. I used the explicit row numbers for that last bit because it's static, I define them more dynamically elsewhere. Just chose this as it's simpler. \$\endgroup\$
    – Mark S.
    Apr 17 at 15:12
  • 2
    \$\begingroup\$ @MarkS. I recommend you leave this question up, apply anything you feel you can get out of it, and then re-post a larger question covering the wider context, unmodified code and your goals/problems. Will avoid un-needed feedback, it also makes the qu more accessible to other people reading the questions and answers. Glad you found my notes useful. \$\endgroup\$
    – Greedo
    Apr 17 at 15:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.