0

I've built this code, and it's working fine. However I expect there must be a more elegant way to embed the range 'c' into the Evaluate function rather than how I've used 'r' to determine the row number, and build that into the reference.

(I'm learning). Copy of (very stripped down) xlsm available here: https://www.dropbox.com/s/e6pcugqs4zizfgn/2018-11-28%20-%20Hide%20table%20rows.xlsm?dl=0

Sub HideTableRows()

Application.ScreenUpdating = False

Dim c As Range
Dim r As Integer

For Each c In Range("ForecastTable[[Group]:[Item]]").Rows
    r = c.Row
    If Application.Evaluate("=COUNTA(B" & r & ":D" & r & ") = 0") = True Then
        c.EntireRow.Hidden = True
    Else: c.EntireRow.Hidden = False
    End If
Next c

Application.ScreenUpdating = True

End Sub
Brian Tompsett - 汤莱恩
  • 5,753
  • 72
  • 57
  • 129
perthling
  • 1
  • 2

3 Answers3

2

There's no specific question/problem, but here's my suggested code improvements.

  • Most notably, I wouldn't execute the Hidden procedure until you have all the rows. That way you don't have repeatedly do something that only need be completed once. This will always be the best practice when looping and manipulating data. Make changes to the sheet AFTER you have identified the range.
  • With the above change, you don't need to turn off ScreenUpdating.
  • The Evaluate function is fine, but isEmpty is probably the best option. There are probably slightly faster methods, perhaps checking multiple if-statements, but that's getting into fractions of a second over thousands of rows (probably not worth researching).
  • Technically you don't really need to loop by rows. You can get by with a single cell in a row, then checking the next two over, see utilization of Offset to generate that range. This also creates a more dynamic than using hard-coded columns ("A"/"B"...etc")
  • Long is recommended over Integer but this is pretty small, and I'm only mentioning it because I posted about it here.. Technically you don't even need it with the above changes.

Here's the code:

Sub HideTableRows()
    Dim c As Range, hIdeRNG As Range, WS As Worksheet

    'based on OP xlsm file.
    Set WS = Sheet4


    'used range outside of used range to avoid an if-statement on every row
    Set hIdeRNG = WS.Cells(Rows.Count, 1)

    'loops through range of single cells for faster speed
    For Each c In Range("ForecastTable[Group]").Cells

        If IsEmpty(Range(c, c.Offset(0, 2))) = 0 Then
            'only need a single member in the row.
            Set hIdeRNG = Union(hIdeRNG, c)
        End If

    Next c

    'Hides rows only if found more than 1 cell in loop
    If hIdeRNG.Cells.Count > 1 Then
        Intersect(WS.UsedRange, hIdeRNG).EntireRow.Hidden = True
    End If

End Sub

Final Thought: There's some major enhancements coming out to Excel supposedly in early 2019 that might be useful for this type of situation if you were looking for a non-VBA solution. Click here for more info from MS.

pgSystemTester
  • 8,979
  • 2
  • 23
  • 49
  • 2
    you are using implicit sheet references throughout and are you sure UsedRange is returning the right range? Only a personal opinion but I find If statement cleaner. – QHarr Nov 28 '18 at 05:03
  • @QHarr good call. It should ref activesheet. I'll clean it up. – pgSystemTester Nov 28 '18 at 05:42
  • Better still tell them to used explicit sheet names if possible – QHarr Nov 28 '18 at 05:52
  • @TinMan I didn't qualify the table at the sheet level for that very reason. Your usedRange interect code is slightly off, but I'll use it. Give you a tick at it's a good concept. – pgSystemTester Nov 28 '18 at 06:29
  • 1
    Oh jeez, next thing I know you’re going to say that the federal department of redundancy department isn’t needed! I gotta shut down. You’re probably right – pgSystemTester Nov 28 '18 at 06:48
  • 1
    FDRD department. That's Good stuff :-) – TinMan Nov 28 '18 at 07:13
  • @PGCodeRider, I like this alternate solution. Definitely a few new things for me to read up on (IsEmpty, Offset, Union, Intersect). One part of my original question however... is there a way to take a Range and embed it into a string? – perthling Nov 29 '18 at 01:16
1

Flipping the logic a bit, why not just filter those three columns for blanks, then hide all the visible filtered blank rows in one go?

Something like this:

Sub DoTheHide()

    Dim myTable As ListObject
    Set myTable = Sheet4.ListObjects("ForecastTable")
    With myTable.Range
        .AutoFilter Field:=1, Criteria1:="="
        .AutoFilter Field:=2, Criteria1:="="
        .AutoFilter Field:=3, Criteria1:="="
    End With

    Dim rowsToHide As Range
    On Error Resume Next
        Set rowsToHide = myTable.DataBodyRange.SpecialCells(xlCellTypeVisible)
    On Error GoTo 0

    myTable.AutoFilter.ShowAllData

    If Not rowsToHide Is Nothing Then
        rowsToHide.EntireRow.Hidden = True
    End If

End Sub
BigBen
  • 46,229
  • 7
  • 24
  • 40
1

Since c is used to iterate over the rows and each row contains the 3 cells in question ("=COUNTA(B" & r & ":D" & r & ") = 0") is equivalent to ("=COUNTA(" & c.Address & ") = 0"). But using the WorksheetFunction directly is a better appraoch.

It should be noted that Range("[Table]") will return the proper result as long as the table is in the ActiveWorkbook. It would be better to useThisWorkbook.Worksheets("Sheet1").Range("[Table]")`.

Sub HideTableRows()

    Application.ScreenUpdating = False

    Dim row As Range, target As Range

    With Range("ForecastTable[[Group]:[Item]]")
        .EntireRow.Hidden = False
        For Each row In .rows
            If Application.WorksheetFunction.CountA(row) = 0 Then
                If target Is Nothing Then
                    Set target = row
                Else
                    Set target = Union(target, row)
                End If
            End If
        Next
    End With

    If Not target Is Nothing Then target.EntireRow.Hidden = True
    Application.ScreenUpdating = True

End Sub
TinMan
  • 6,624
  • 2
  • 10
  • 20