debugMinor
Count rows of a table
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 FunctionSolution
Just some things that jump out at me:
You don't reset
Huge red flag right there. If you're going to change
Specifically: (Square brackets for emphasis)
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
Scoping
Your function is implicitly public. You should explicitly set a
Integer
Is officially deprecated. The compiler will silently convert all
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
You don't reset
OnErrorHuge 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 CleanExitYour 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 FunctionScoping
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 CleanExitOption 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 FunctionContext
StackExchange Code Review Q#125462, answer score: 5
Revisions (0)
No revisions yet.