HiveBrain v1.2.0
Get Started
← Back to all entries
debugMinor

Count rows of a table

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
countrowstable

Problem

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

Solution

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.

Code Snippets

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
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

Context

StackExchange Code Review Q#125462, answer score: 5

Revisions (0)

No revisions yet.