Advanced multiple criteria filter VBA

Question

This code is working on the user selection when pressed a button the table will be filtered by the number of criteria the user selected:

    Sub advancedMultipleCriteriaFilter()

 Dim cellRng As Range, tableObject As Range, subSelection As Range
 Dim filterCriteria() As String, filterFields() As Integer
 Dim i As Integer

Application.ScreenUpdating = False
'Call removeSpace


If Selection.Rows.Count > 1 Then
 MsgBox "Cannot apply filter to multiple rows within the same column. Please make another selection and try again.", vbInformation, "Selection Error!"
 Exit Sub
 End If


i = 1
 ReDim filterCriteria(1 To Selection.Cells.Count) As String
 ReDim filterFields(1 To Selection.Cells.Count) As Integer

Set tableObject = Selection.CurrentRegion
 For Each subSelection In Selection.Areas
 For Each cellRng In subSelection
 filterCriteria(i) = cellRng.Text
 filterFields(i) = cellRng.Column - tableObject.Cells(1, 1).Column + 1
 i = i + 1
 Next cellRng
 Next subSelection

With tableObject
 For i = 1 To UBound(filterCriteria)
 .AutoFilter field:=filterFields(i), Criteria1:=filterCriteria(i)
 Next i
 End With

Call GetLastRow

Set tableObject = Nothing
Application.ScreenUpdating = True

End Sub

Sub resetFilters()

Dim sht As Worksheet
Dim LastRow As Range

Application.ScreenUpdating = False

   On Error Resume Next
        If ActiveSheet.FilterMode Then
    ActiveSheet.ShowAllData
  End If

'Range("A3:T3").ClearContents
Application.ScreenUpdating = True
Call GetLastRow

End Sub

Private Sub GetLastRow()

 'Step 1: Declare Your Variables.
  Dim LastRow As Long

 'Step 2: Capture the last used row number.
  LastRow = Cells(Rows.Count, 8).End(xlUp).Row

 'Step 3: Select the next row down
  Cells(LastRow, 8).Offset(1, 0).Select

 End Sub

Answer 1

Private Sub GetLastRow()

I like that it's Private. Other than that... a Sub doesn't "get" anything; a Sub does something - this is a side-effecting procedure with the lipstick of a Function. A pig with lipstick is still a pig though. Let's look at how it's implemented:

 'Step 1: Declare Your Variables.
  Dim LastRow As Long

 'Step 2: Capture the last used row number.
  LastRow = Cells(Rows.Count, 8).End(xlUp).Row

 'Step 3: Select the next row down
  Cells(LastRow, 8).Offset(1, 0).Select

These comments are noise. Comments that say what the code does are useless clutter, unless the code is absolutely unreadable... in which case the solution is to improve the code's readability. Good comments say why, not what. Bad comments should be removed.

There are a number of serious issues with that procedure. Cells and Rows both implicitly refer to the active worksheet, which means if the user happens to activate another sheet while the code is running, everything falls apart (I see you've addressed that elsewhere though) - it also falls apart if other code activates another sheet than the one that's expected to be active though. And given how your entire code [ab]uses Select and Activate, that's a real concern.

This would be my GetLastRow:

Private Function GetLastRow(ByVal sheet As Worksheet, Optional ByVal fromColumn As Long = 1) As Long
    GetLastRow = sheet.Cells(sheet.Rows.Count, fromColumn).End(xlUp).Row
End Function

No side effects. Returns ("gets") a value. Says what it does, does what it says: it returns the last row; a better name for a side-effecting procedure that selects the first empty row could be SelectFirstNonEmptyRowInColumnH. Too specific? It's exactly what it does though. The problem isn't the specific name - the problem is what the procedure does.

The benefits of using non-side-effecting functions are numerous: first, I can call my function off any worksheet (not just the active one), make it look at any column, and I can call it 20 times in a row on 20 different sheets, not once will my Application.Selection be affected.

It also makes code that reads exactly like what it does:

Call GetLastRow

Regardless of the Call keyword being useless here (or anywhere else - it's been obsolete since the advent of the implicit procedure call syntax... over 20 years ago), reading the above line of code has a very different feel than reading this one:

sheet.Cells(GetLastRow(sheet, 8), 8).Offset(1).Select

More complicated? Ok then, let's split it up:

Const importantColumn As Long = 8

Dim lastRow As Long
lastRow = GetLastRow(sheet, importantColumn)

sheet.Cells(lastRow, importantColumn) _
     .Offset(1) _
     .Select

By not hiding .Select operations behind a side-effecting procedure call [disguised as a function], we make it clear that we're tampering with the workbook's state.

---

There's only ever ONE reason to use .Select in VBA code: when you want to set the current selection for the user.

Using .Select for any other reason, is asking for your code to be as slow as Excel VBA code can get, not to mention extremely frail and bug-prone.

Working off Selection is dangerous. In fact, your code can very easily blow up on the 2nd executable statement:

If Selection.Rows.Count > 1 Then

How? You assume Selection is a Range. But Selection can be a Shape. Selection can be a Chart, or a Series. Selection can be Nothing. Each of these make execution jump to your ErrorHandler subroutine, to display a cryptic error message in the debug/immediate pane, that doesn't tell anything about what's going on, where, or why:

Error number: 91 Object variable or With block variable not set.

Or

Error number: 438 Object doesn't support this property or method.

But programmers making assumptions (Selection is a Range?) are a dangerous thing: you'll likely never encounter these error messages, because when you test your macro yourself, Selection will be a Range. Things get more fun when it's an actual user running your code though. Users are particularly bad at wording useful bug reports - most of the time they sound like a downvoted Stack Overflow question that's accumulating "unclear what you're asking" close votes:

I clicked your macro button but nothing happens, it's not working.

Good luck figuring that one out!

Instead, you could separate responsibilities, increase the level of abstraction, and have smaller procedures that do one thing - and have few reasons to fail. Then you could include the procedure's name in your Debug.Print statement, and at least you'd know which step blew up.

---

I'd review the actual crux of the code, but the [lack of] indentation makes my head spin, so instead I'll point you to Rubberduck and its Smart Indenter tool (disclaimer: I manage this open-source project; credits to @Comintern for most of the work on the Smart Indenter port), which can help you fix this.

Please read: How to avoid using Select in Excel VBA macros.

Answer 2

1. Use indentation consistently. It improves readability, ease finding mistakes...
   You can read more e.g. here
   e.g.:

---

Sub advancedMultipleCriteriaFilter()

    Dim cellRng As Range, tableObject As Range, subSelection As Range
    Dim filterCriteria() As String, filterFields() As Integer
    Dim i As Integer

    On Error GoTo ErrorHandler

    Application.ScreenUpdating = False

    If Selection.Rows.Count > 1 Then
        MsgBox "Cannot apply filter to multiple rows within the same column. Please make another selection and try again.", vbInformation, "Selection Error!"
        Exit Sub
    End If

    i = 1
    ReDim filterCriteria(1 To Selection.Cells.Count) As String
    ReDim filterFields(1 To Selection.Cells.Count) As Integer

    Set tableObject = Selection.CurrentRegion
    For Each subSelection In Selection.Areas
        For Each cellRng In subSelection
            filterCriteria(i) = cellRng.Text
            filterFields(i) = cellRng.Column - tableObject.Cells(1, 1).Column + 1
            i = i + 1
        Next cellRng
    Next subSelection

    With tableObject
        For i = 1 To UBound(filterCriteria)
            .AutoFilter field:=filterFields(i), Criteria1:=filterCriteria(i)
        Next i
    End With

    Call GetLastRow

    Set tableObject = Nothing
    Application.ScreenUpdating = True

    Exit Sub
ErrorHandler:
    Debug.Print "Error number: " & Err.Number & " " & Err.Description

End Sub

2. naming conventions! Your names should be self explaining, easy to understand, consistent and correct, e.g.
   • tableObject: why do you call it object, and all the others are Rng?
   • GetLastRow: "get ..." is generally name of a function, it should be SelectLastRow, MoveSelectionToLastRow...
   • SubSelection, CellRng: CurrentArea, CurrentCell?