1
\$\begingroup\$

I need to get a table from an Excel worksheet. Sometimes I know the worksheet that contains the table, other times I might know it's going to be in one of a handful of worksheets, and still other times I might not even know what workbook it's in.

To me, this seemed like a good place to use an overloads function. What I did was create the following overloaded function which recursively calls itself to dig down into each "level" of the input object's hierarchy so that it allows me to look for the table using a variety of input objects.

I'd like to get some feedback on this solution; is it acceptable practice? Is this a good use of overloading? Is there a better (more practical, more efficient, etc.) way to do it?

Note 1: I've omitted class members not related to this question.

Note 2: I haven't gotten deep enough into debugging to add any specific error handling to the try/catch structure, but I do plan to add more specific "catches" eventually.

Imports Microsoft.Office
Imports Microsoft.Office.Interop.Excel
Imports System.Windows.Forms
Imports System.Runtime.InteropServices

Public Class MyExcel
    Public Overloads Function GetTable(ByVal SearchName As String, ByVal SearchSheets() As Worksheet, Optional ByVal ExactMatch As Boolean = False) As ListObject
        If Not ExactMatch Then SearchName = "*" & SearchName & "*"
        Try
            For Each ws As Worksheet In SearchSheets
                For Each lo As ListObject In ws.ListObjects
                    If lo.Name Like SearchName Then Return lo
                Next
            Next
        Catch ex As Exception
            MessageBox.Show(ex.ToString)
        End Try
        Return Nothing
    End Function
    Public Overloads Function GetTable(ByVal SearchName As String, ByVal SearchBooks() As Workbook, Optional ByVal ExactMatch As Boolean = False) As ListObject
        Dim loFound As ListObject
        Dim wsList As New List(Of Worksheet)
        Try
            For Each wb In SearchBooks
                For Each ws In wb.Worksheets
                    wsList.Add(ws)
                Next
                loFound = Me.GetTable(SearchName:=SearchName, SearchSheets:=wsList.ToArray, ExactMatch:=ExactMatch)
                If Not loFound Is Nothing Then Return loFound
            Next
        Catch ex As Exception
            MessageBox.Show(ex.ToString)
        End Try
        Return Nothing
    End Function
    Public Overloads Function GetTable(ByVal SearchName As String, ByVal SearchApps() As Microsoft.Office.Interop.Excel.Application, Optional ByVal ExactMatch As Boolean = False) As ListObject
        Dim loFound As ListObject
        Dim wbList As New List(Of Workbook)
        Try
            For Each app In SearchApps
                For Each wb In app.Workbooks
                    wbList.Add(wb)
                Next
                loFound = Me.GetTable(SearchName:=SearchName, SearchBooks:=wbList.ToArray, ExactMatch:=ExactMatch)
                If Not loFound Is Nothing Then Return loFound
            Next
        Catch ex As Exception
            MessageBox.Show(ex.ToString)
        End Try
        Return Nothing
    End Function
End Class
\$\endgroup\$
2
\$\begingroup\$

Basically, this isn't recursion but just overloading methods being called from each other, which is a valid approach targeting the problem.

But nevertheless, you could do better.

  • Naming

    Based on the naming guidelines method parameters should be nameed using camelCase casing.
    If you have own naming guidelines you for sure should stick to them.

    Abbreviations for naming variables like loFound doesn't tell anything at all. You should name your variable as meaningful as possible. In that way Sam the maintainer won't have a hard time evaluating the purpose of the variable. That being said, I would have expected a name like foundTable inside a method named GetTable().

  • Scope

    You should declare your variables as near as possible to their usage. It does not help readability if you declare them at the top of the method if you use them way down the method.

  • Using the right type

    It doesn't make sense to use a List<T> to collect items and then calling ToArray() because the method to be called takes an array as parameter, at least if you can change it.

    I would like to suggest changing the parameter to IEnumerable<T> because you are only iterating over the array which can be done in the same way with an IEnumerable<T> and much better, if you want to pass an array, a List<T> or a ICollection<T> you can still do it.

    You should always try to code against interfaces and not against a concrete implementation.

  • Spacing

    By adding vertical space to logicaly related code your code will improve readability.


So let us take a look at this

Public Overloads Function GetTable(ByVal SearchName As String, ByVal SearchBooks() As Workbook, Optional ByVal ExactMatch As Boolean = False) As ListObject
   Dim loFound As ListObject
   Dim wsList As New List(Of Worksheet)
   Try
       For Each wb In SearchBooks
           For Each ws In wb.Worksheets
               wsList.Add(ws)
           Next
           loFound = Me.GetTable(SearchName:=SearchName, SearchSheets:=wsList.ToArray, ExactMatch:=ExactMatch)
           If Not loFound Is Nothing Then Return loFound
       Next
   Catch ex As Exception
       MessageBox.Show(ex.ToString)
   End Try
   Return Nothing
End Function

The List<T> class has a method AddRange() to add a range of objects. This would improve the method regarding performance.

But you would gain lot more performance if you would declare the List<T> just inside the outer loop. Why do you add all of the WorkSheets but never clear the List ? You will iterate over the whole list each time the searched item isn't found.

So let us do this right. Because the Sheets interface implements IEnumerable we can use the overloaded constructor of List<T> and pass the IEnumerable like so

Public Overloads Function GetTable(ByVal searchName As String, ByVal searchBooks As IEnumerable(Of Workbook), Optional ByVal exactMatch As Boolean = False) As ListObject()

    Try
        For Each wb In searchBooks
            Dim sheets As New List(Of Worksheet)(wb.Sheets.OfType(Of Worksheet))

            Dim foundTable As ListObject = Me.GetTable(searchName:=searchName, searchSheets:=sheets, exactMatch:=exactMatch)
            If Not foundTable Is Nothing Then Return foundTable

        Next
    Catch ex As Exception
        MessageBox.Show(ex.ToString)
    End Try
    Return Nothing
End Function
\$\endgroup\$
  • \$\begingroup\$ That looks much simpler and more efficient. You are right, I never did clear the list before! Sorry!! I did not know you could declare a variable within a loop like that - I guess i just figured it would throw some run-time error on the second pass about trying to declare a variable that already exists. I will try your suggestions. I especially like changing the parameter to the IEnumberable. The advice to code to interfaces rather than implemtations is well received! Thanks! \$\endgroup\$ – CBRF23 Jul 13 '15 at 15:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.