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:
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 Integer
s to Long
s, 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.