7
\$\begingroup\$

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
        .Filters.Add "Excel Document", ("*.csv, *.xls*")
        .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
\$\endgroup\$
4
\$\begingroup\$

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
        .Filters.Add "Excel Document", ("*.csv, *.xls*")
        .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.

\$\endgroup\$
4
  • \$\begingroup\$ Thanks. Why the change in error handling? I will look into the model-view-controller pattern you mentioned! \$\endgroup\$ – Raystafarian Feb 18 '17 at 18:31
  • 2
    \$\begingroup\$ Updated the answer to include my point of view on error handling in VBA. \$\endgroup\$ – Leviathan Feb 18 '17 at 22:06
  • \$\begingroup\$ 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! \$\endgroup\$ – Raystafarian Feb 21 '17 at 19:11
  • 1
    \$\begingroup\$ 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? :-( \$\endgroup\$ – Leviathan Feb 21 '17 at 19: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.