This is a cleaned-up version I would suggest:
Private Function ImportDataFromExternalSource(ByVal boolPickSheet As Boolean, Optional ByVal lngNumberOfColumns As Long = 0) As Variant
Dim lngLastRow As Long
Dim strFileName As String
Dim xlApp As New Excel.Application
Dim wbkTarget As Excel.Workbook
Dim shtTarget As Excel.Worksheet
' ask the user to select a file and try to open it with Excel
Set xlApp = New Excel.Application
Do While wbkTarget Is Nothing
strFileName = FilePicker()
If strFileName = "" Then Exit Function ' user requested abort
On Error Resume Next
Set wbkTarget = xlApp.Workbooks.Open(strFileName)
If Err <> 0 Then MsgBox "An error occurred while opening the file" & vbNewLine _
& strFileName & vbNewLine _
& vbNewLine _
& Err.Description _
On Error GoTo 0
Set shtTarget = wbkTarget.Sheets(1)
' let the user select the sheet to import from
If boolPickSheet Then
xlApp.Visible = True
Set shtTarget = xlApp.InputBox("Pick a cell on the sheet you would like to import", Type:=8).Parent
' determine the data that should be imported
If lngNumberOfColumns = 0 Then lngNumberOfColumns = shtTarget.Cells(1, Columns.Count).End(xlToLeft).Column
lngLastRow = shtTarget.Cells(Rows.Count, 1).End(xlUp).Row
ImportDataFromExternalSource = shtTarget.Range(shtTarget.Cells(1, 1), shtTarget.Cells(lngLastRow, lngNumberOfColumns)) ' convert Range to Array
Public Function FilePicker() As String
.AllowMultiSelect = False
.Title = "Select the file with your data."
.Filters.Add "Excel Document", ("*.csv, *.xls*")
.InitialView = msoFileDialogViewDetails
If .Show Then FilePicker = .SelectedItems(1)
Being called like this:
Public Sub Test()
Dim x As Variant
x = ImportDataFromExternalSource(False, 1)
If IsEmpty(x) Then
MsgBox "you've cancelled", vbInformation
If you are interested in the reasons behind the changes I made, I can provide some detailed information if you like.
Issues that still needs to be addressed
It would be preferable to decouple the user interaction (file and sheet selection) from the import logic, known as the Model-view-controller pattern. Have one function asking the user for the file and (optionally) the sheet, returning a
Worksheet object. Now you can feed that object to another function that retrieves the array from the given sheet.
This way you are more flexible if, for example, you decide at one point that the sheet should also be able to be automatically retrieved by some other means. Just implement that in a way that it returns a
Worksheet object, and you are good to go.
The way you determine the last cell in the sheet will fail in some circumstances (e.g. if the sheet is empty). Have a look at https://stackoverflow.com/a/11169920/6216216 for a thourough discussion of more robust alternatives.
Notes on Error Handling
You switched error handling on at the beginning of the function - as it is often recommended. There is no good way to do proper error handling in VBA (one of VBA's bigger flaws), but I thoroughly dislike the variant of catching all errors and handling them in one monolithic error handling routine at the end of the function/sub.
This is why:
- The handling of an error gets disconnected from the origin of the error. This makes the code harder to read and easier to mess up during maintanance. Furthermore, debugging becomes a pain.
- You may cast too broad a net, you might catch errors that you didn't want to catch.
Especially the second issue can become a serious problem when you start handling unexpected errors the same way as expected errors, since the error handling routine has no way to tell them apart.
Exactly this happened in your original code: If you call
ImportDataFromExternalSource(False) and you cancel the file picker,
fileName contains the empty string
"". Trying to open that as an Excel workbook will fail and the error handler is called. This seems to be expected, because a message is displayed and the program gracefully terminated by jumping to the
CleanExit label. During cleanup however,
targetBook.Close is called, which will fail, because there is no
targetBook, and an unexpected error is raised. Since your error handler cannot distinguish this from the expected error, the same routine is called: the messagebox is displayed again, the code continues with the
CleanExit label and... now you are stuck in an infinite loop, because
targetBook.Close will again raise an error.
The paradigm I followed is to apply the error handler to as litte code as possible, that is, the statements that I expect to fail. In this case, that is the
Workbooks.Open function. The error handling code follows directly afterwards. If the handling code is more complex you should move it after the
On Error GoTo 0, otherwise errors occuring in that code would just be silently ignored.
Do the same to other parts of the function where you expect errors (generally speaking; there aren't any in this code), and you have a clean separation of error handling for expected and unexpected errors: Expected errors are handled right away where they occurred, unexpected errors are bubbled up the calling stack until some code handles them that is better suited to do so. The worst case would be a runtime error that is displayed to the user indicating what exactly went wrong, but that would be preferable in most cases to exhibiting undefined behavior (as the infinite loop in your code).
This way your code becomes more robust, fast-failing if something went wrong instead of hiding the error and resulting in arbitrary consequences.