Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am still trying to wrap my head around good programming practice and just wrote this routine to read in an Excel file and return all the sheets into a dataset:

Public Shared Function ReadExcelIntoDataSet(ByVal FileName As String, Optional ByVal TopRowHeaders As Boolean = False) As DataSet
    Try
        Dim retval As New DataSet

        Dim strConnString As String = "Provider=Microsoft.ACE.OLEDB.12.0;Data Source=" & _
                                        FileName & ";Extended Properties=""Excel 12.0;IMEX=1;" & _
                                        "HDR=" & If(TopRowHeaders, "Yes", "No") & """;"

        Using oleExcelConnection = New OleDb.OleDbConnection(strConnString)
            oleExcelConnection.Open()

            Using oleExcelCommandString = New OleDb.OleDbCommand("", oleExcelConnection)

                Using oleExcelAdapter = New OleDb.OleDbDataAdapter(oleExcelCommandString)
                    Dim dtSchema As DataTable = oleExcelConnection.GetOleDbSchemaTable(OleDb.OleDbSchemaGuid.Tables, Nothing)
                    For Each dr As DataRow In dtSchema.Rows
                        Dim Sheetname As String = dr.Item("TABLE_NAME").ToString

                        If Sheetname.EndsWith("$") Then
                            oleExcelCommandString.CommandText = "Select * From [" & Sheetname & "]"
                            oleExcelAdapter.Fill(retval, Sheetname)
                        End If
                    Next

                    Return retval

                End Using

            End Using

        End Using

    Catch ex As Exception
        Dim ErrorData As String = "Unable to Read in Data from the Excel File " & FileName & vbCrLf & _
                                   "Module: Utility > ReadExcelIntoDataSet"
        Throw New Exception(ErrorData & vbCrLf & "ERROR Data:" & vbCrLf & ex.Message)
    End Try
End Function

My first questions are:

  1. Is the use of the Using statements good or is it some kind of over-kill and should this maybe be handled in a Finally statement in stead?
  2. Is this the best method to read in an Excel File (Given that they can be either .xls or .xlsx) in the fastest possible way?
  3. What is the best way to pass the error up to the calling code? Did my Catch statement do that well enough??
share|improve this question

1 Answer 1

up vote 2 down vote accepted
  1. The Using is good practice and not overkill. Trying to dispose the resources in the Finally part has the disadvantage, that you don't know when the exception happened and which resources have been assigned by that time. The Using statement does this automatically for you internally: If resouce <> Nothing Then resource.Dispose() End If

  2. You need different connection strings for the different Excel types. You can use this function:

Private Function GetConnectionString(ByVal filename As String) As String
    Const xlsConnString As String = "Provider=Microsoft.Jet.OLEDB.4.0;Data Source={0};Extended Properties=Excel 8.0;"
    Const xlsxConnString As String = "Provider=Microsoft.ACE.OLEDB.12.0;Data Source={0};Extended Properties=Excel 12.0;"

    If Path.GetExtension(filename).Length = 5 Then ' .xlsx, .xltx
        Return String.Format(xlsxConnString, filename)
    Else ' .xls, .xlt
        Return String.Format(xlsConnString, filename)
    End If
End Function
  1. You can either propagate the exception or return Nothing. In my opinion, your version does it very well.
share|improve this answer
    
Thank you for such a detailed response!!! –  John Bustos Jan 10 '13 at 20:11
    
... One more question, if you'd be willing to answer it - It seems that the ACE.OLEDB connection opens the .xls files too... Is there any reason to use the Jet provider?? –  John Bustos Jan 10 '13 at 20:12
    
If it does it's okay! I supported only xls first and added support for xlsx later in one of my projects and did not realize that ACE.OLEDB supports the older format as well. –  Olivier Jacot-Descombes Jan 10 '13 at 20:16
    
Thanks!!! - Makes sense :) –  John Bustos Jan 10 '13 at 20:16

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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