# Get data from user input as workbook or worksheet

I had two different functions for populating a data array. I tried to refactor them using a single argument as to whether or not the user should be selecting a worksheet or just the workbook.

The primary issue being whether I need to prompt the user to select a range or not. The main difference between the two before was if using just a sheet I didn't need xlApp.

I came at the refactoring a little backwards, so I imagine it can be improved.

Option Explicit
Private Function ImportDataFromExternalSource(ByVal pickSheet As Boolean, Optional ByVal numberOfColumns As Long = 0) As Variant
Dim lastRow As Long
Dim fileName As String
Dim xlApp As New Application
Set xlApp = New Excel.Application
Dim targetBook As Workbook
Dim targetSheet As Worksheet
Dim targetDataRange As Range
On Error GoTo ErrorHandler
fileName = File_Picker

Set targetBook = xlApp.Workbooks.Open(fileName)
Set targetSheet = targetBook.Sheets(1)

If pickSheet Then
xlApp.ActiveWorkbook.Windows(1).Visible = True
xlApp.Visible = True
targetBook.Activate
targetBook.Sheets(1).Activate
Set targetDataRange = xlApp.InputBox("Pick a cell on the sheet you would like to import", Type:=8)
Set targetSheet = targetDataRange.Parent
End If

If numberOfColumns = 0 Then numberOfColumns = targetSheet.Cells(1, Columns.Count).End(xlToLeft).Column
lastRow = targetSheet.Cells(Rows.Count, 1).End(xlUp).Row
ImportDataFromExternalSource = targetSheet.Range(targetSheet.Cells(1, 1), targetSheet.Cells(lastRow, numberOfColumns))

CleanExit:
If pickSheet Then
xlApp.Quit
Exit Function
End If
ThisWorkbook.Activate
targetBook.Close
Exit Function

ErrorHandler:
MsgBox "you've cancelled"
Resume CleanExit
End Function

Public Function File_Picker() As String
Dim workbookName As String
Dim selectFile As FileDialog
Set selectFile = Application.FileDialog(msoFileDialogOpen)
With selectFile
.AllowMultiSelect = False
.Title = "Select the file with your data."
.Filters.Clear
.InitialView = msoFileDialogViewDetails
If .Show Then File_Picker = .SelectedItems(1)
End With
Set selectFile = Nothing
End Function


I was using this little procedure to test -

Public Sub test()
Dim x As Variant
x = ImportDataFromExternalSource(False, 1)
Debug.Print x(1)
End Sub


This is a cleaned-up version I would suggest:

Option Explicit

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 _
, vbCritical
On Error GoTo 0
Loop
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
End If

' 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

' cleanup
wbkTarget.Close False
xlApp.Quit
End Function

Public Function FilePicker() As String
With Excel.Application.FileDialog(msoFileDialogOpen)
.AllowMultiSelect = False
.Title = "Select the file with your data."
.Filters.Clear
.InitialView = msoFileDialogViewDetails
If .Show Then FilePicker = .SelectedItems(1)
End With
End Function


Being called like this:

Public Sub Test()
Dim x As Variant

x = ImportDataFromExternalSource(False, 1)
If IsEmpty(x) Then
MsgBox "you've cancelled", vbInformation
Else
Debug.Print x(1)
End If
End Sub


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

1. 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.

2. 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.

• Thanks. Why the change in error handling? I will look into the model-view-controller pattern you mentioned! – Raystafarian Feb 18 '17 at 18:31
• Updated the answer to include my point of view on error handling in VBA. – Leviathan Feb 18 '17 at 22:06
• Thanks for the explanation on error handling. I'm going to use your example here. One thing to note is if the user cancels out of picking a range you get an object error, so wrapping that in a different error handler is a good idea! – Raystafarian Feb 21 '17 at 19:11
• Ah, yeah, you are right; that's a good example for another case to wrap in an error handler. One thing I did neglect to mention is that moving the error handling code after the On Error GoTo 0 is quite unconvenient because the Err object gets wiped, you don't have a way to determine if and what error occurred anymore. You have to introduce a new variable that you set with the error code before the On Error GoTo 0 so that you can retrieve it thereafter. Did I mention that error handling in VBA is messy? :-( – Leviathan Feb 21 '17 at 19:39