Count rows of a table

Question

I am trying to count the rows of two tables generated by SQL connections in Excel VBA. My plan was to use ListObject.DataBodyRange.Rows.Count however, this errors when a table is empty, which one often is. I found it completely impossible to use On Error GoTo ... twice within one sub so I pulled out the counts as functions like this. I based my error handling pattern on this answer. Is this the best way to go?

Function CountCurrencyTrades() As Integer

    Dim sheetCurrencyTable As Worksheet
    Set sheetCurrencyTable = Worksheets("CurrencyTable")

    Dim tblCurrencyTrades As ListObject
    Set tblCurrencyTrades = sheetCurrencyTable.ListObjects("CurrencyQuery")
    On Error GoTo CurrencyTradesEmpty
    CountCurrencyTrades = tblCurrencyTrades.DataBodyRange.Rows.Count

CleanExit:

    Exit Function

CurrencyTradesEmpty:

    CountCurrencyTrades = 0
    Resume CleanExit

End Function

Answer 1

Just some things that jump out at me:

---

You don't reset OnError

Huge red flag right there. If you're going to change OnError Goto you should immediately write the corresponding lines to set it back to normal.

Specifically: (Square brackets for emphasis)

    On Error GoTo CurrencyTradesEmpty
    CountCurrencyTrades = tblCurrencyTrades.DataBodyRange.Rows.Count
    [On Error Goto 0] 

CleanExit:

    Exit Function

CurrencyTradesEmpty:
    [On Error Goto 0] 
    CountCurrencyTrades = 0
    Resume CleanExit

---

Your function has too many responsibilities

Single Responsibility Principle. Your function has a hardcoded worksheet, hardcoded listObject and then handles the counting of said object. I would have the function take a ListObject as an argument and let another function handle the retrieval of said objects. Like so:

    Option Explicit

    Public Function GetCurrencyTradesCount() As Long

        Dim sheetCurrencyTable As Worksheet
        Set sheetCurrencyTable = Worksheets("CurrencyTable")

        Dim tblCurrencyTrades As ListObject
        Set tblCurrencyTrades = sheetCurrencyTable.ListObjects("CurrencyQuery")

        GetCurrencyTradesCount = CountListRows(tblCurrencyTrades)

    End Function

    Private Function CountListRows(ByRef targetList As ListObject) As Long
        '/ On Error (I.E. empty list), returns 0

        On Error GoTo CleanFail
        CountListRows = targetList.DataBodyRange.Rows.Count
        On Error GoTo 0

CleanExit:
        Exit Function

CleanFail:
        On Error GoTo 0
        CountListRows = 0
        Resume CleanExit

    End Function

Scoping

Your function is implicitly public. You should explicitly set a Public or Private scope for it.

---

Integer

Is officially deprecated. The compiler will silently convert all Integers to Longs, so just use Long instead.

---

Error Handling

Do you really want your function to return 0 for all errors? What if it's given an empty object reference? I would rename your label to CleanFail: and think about what other failure scenarios it might want to deal with.

Answer 2

You can avoid the extra goto, and use inline error resetting, like this:

Function CountCurrencyTrades() As Integer

    Dim sheetCurrencyTable As Worksheet
    Set sheetCurrencyTable = Worksheets("CurrencyTable")

    Dim tblCurrencyTrades As ListObject
    Set tblCurrencyTrades = sheetCurrencyTable.ListObjects("CurrencyQuery")

    CountCurrencyTrades = 0
    On Error Resume Next
    CountCurrencyTrades = tblCurrencyTrades.DataBodyRange.Rows.Count    
    On Error GoTo 0

End Function