Get Workbook Method(s)

Question

I'm re-writing my Module of standard methods. These 3 are used to retrieve Workbooks as Workbook Objects. The standard I'm aiming for here is "Third-party Library/Add-in". So:

• Do these functions adhere to the principle of least surprise?
• Have I missed any potential error cases?

As always, general feedback is also welcome.

Public Function GetWorkbook(ByVal strFileName As String, Optional ByVal strFilePath As Variant) As Workbook

        If Not (IsMissing(strFilePath) Or strFilePath.TypeName = "String") Then
            PrintErrorMessage "File Path was not supplied as a string", stopExecution:=True
        End If

    Dim wbIsOpen As Boolean

        wbIsOpen = IsWorkbookOpen(strFileName)
        If wbIsOpen Then
            Set GetWorkbook = Workbooks(strFileName)
        Else
            If Not IsMissing(strFilePath) Then
                Set GetWorkbook = OpenWorkbook(strFilePath & strFileName)
            Else
                PrintErrorMessage "Workbook (" & strFileName & ") is not open, and no filepath was supplied", stopExecution:=True
            End If
        End If

End Function

Public Function IsWorkbookOpen(ByVal strTargetName As String) As Boolean

    Dim wbTest As Workbook

        On Error Resume Next
            Set wbTest = Workbooks(strTargetName)
            IsWorkbookOpen = (wbTest.Name = strTargetName)
        On Error GoTo 0

End Function

Public Function OpenWorkbook(ByVal strFileName As String, strFilePath As String) As Workbook

    Dim strFullFilename As String
        strFullFilename = strFilePath & strFileName

    Dim wbOpenedSuccessfully As Boolean

        On Error Resume Next
            Set OpenWorkbook = Workbooks.Open(strFullFilename)
            wbOpenedSuccessfully = IsWorkbookOpen(strFileName)
        On Error GoTo 0

        If Not wbOpenedSuccessfully Then
            PrintErrorMessage "Failed to open workbook """ * strFullFilename & """", stopExecution:=True
        End If

End Function

Answer 1

Why is strFilePath an optinal parameter, if GetWorkbook does not work without a path? And if a missing path triggers your stopExecution:=True why check it later a second time?

Public Function GetWorkbook(strFileName As String, strFilePath As String) As Workbook

  If IsWorkbookOpen(strFileName) Then
    Set GetWorkbook = Workbooks(strFileName)
  ElseIf strFilePath = "" Then
    PrintErrorMessage "File Path was not supplied as a string", stopExecution:=True
  Else
    Set GetWorkbook = OpenWorkbook(strFilePath, strFileName)
  End If

End Function

Public Function IsWorkbookOpen(ByVal strTargetName As String) As Boolean

  On Error Resume Next
    'If Len(Workbooks(strTargetName).Name) = 0 Then IsWorkbookOpen = False Else IsWorkbookOpen = True
    IsWorkbookOpen = (Len(Workbooks(strTargetName).Name) > 0)
    Err.Clear
  On Error GoTo 0

End Function

Public Function OpenWorkbook(ByVal strFileName As String, strFilePath As String) As Workbook

  On Error Resume Next
  Set OpenWorkbook = Workbooks.Open(strFilePath & strFileName)

  If Err.Number Or Not IsWorkbookOpen(strFileName) Then
    PrintErrorMessage "Failed to open workbook """ & strFullFilename & """", stopExecution:=True
  End If

  Err.Clear
  On Error GoTo 0

End Function

I am no friend of declaring variables which will be used only once. If there is really need for, then better add a comment to the code. Which makes wbIsOpen, wbTest, strFullFilename and wbOpenedSuccessfully obsolete for my eye...

And as always: try to avoid On Error Resume Next as much as possible. Errors should pop a message and not be part of normal coding behavior. There lots of non-error ways to get that information... :/

Hint: There is a typo at your OpenWorkbook, at the error-message there is a * which probably should be a &.

Just to sum up the comments... doing it in one function, is that an option?

Public Function GetWorkbook(strFileName As String, Optional strFilePath As String) As Workbook

  On Error Resume Next
  If Len(Workbooks(strTargetName).Name) = 0 Then

    Err.Clear
    On Error Goto 0

    If Len(strFilePath) then
      Set GetWorkbook = OpenWorkbook(strFilePath & strFileName)
    End If

  Else

    On Error Goto 0
    Set GetWorkbook = Workbooks(strFileName)

  End If

End Function

To only check if the wb is open you just could use Not GetWorkbook("workbookname") is Nothing to get a true if it is present...

Still I don't like the use of errors and always would would prefer something like this: (It would work the same way but without use of errors)

Public Function GetWorkbookByName(strWorkbook As String, Optional strPath As String) As Workbook
  Dim MyWorkbook As Workbook

  For Each MyWorkbook In Workbooks
    If MyWorkbook.name = strWorkbook Then

      Set GetWorkbookByName = MyWorkbook
      Exit Function

    End If
  Next

  If Len(strPath) Then
    Set GetWorkbookByName = Workbooks.Open(strPath & strWorkbook)
  End If

End Function

But unfortunately that doesn't work for older versions and it just doesn't look nice to fetch every process...

Answer 2

I will not mention again that I find the outdenting of Dim statements hurt readability. Instead I'll focus on this part:

On Error Resume Next
    Set wbTest = Workbooks(strTargetName)
    IsWorkbookOpen = (wbTest.Name = strTargetName)
On Error GoTo 0

What error are you expecting to trap here?

Workbooks(strTargetName)

This will raise runtime error 9 - Subscript out of range. By then you already know everything you need to know.

(wbTest.Name = strTargetName)

This will raise runtime error 91 - Object variable or With block variable not set - but you already know if that call is going to work before you even try it.

This would be cleaner - slightly overkill with error-handling, but cleaner:

Public Function IsWorkbookOpen(ByVal wbFullFileName As String) As Boolean

    On Error GoTo CleanFail
    IsWorkbookOpen = Not Workbooks(wbFullFileName) Is Nothing

CleanExit:
    Exit Function
CleanFail:
    Err.Clear
    Resume CleanExit
End Function

Notice the difference between strTargetName and wbFullFileName: "str" says "that's a string". "wb" says "that's something about a workbook". Both are Hungarian, only one of them is doing it right. Avoid prefixes that indicate the type.

Answer 3

Optional ByVal strFilePath As Variant

Why are you allowing this to be a Variant? Why not simply require a string type (?). This eliminates the already awkward If Not (IsMissing or) comparison too. You can do a <> "" check on your strFilePath variable instead of the awkwardness IsMissing introduces.

If you for some reason need to know a situation where the method is called with no argument vs an empty string then leave the IsMissing check, but I suspect that you are mainly concerned with "was a valid path passed?"

This allows a much cleaner function.

Note I got rid of the wbIsOpen variable. I also fixed a compilation error in how you call OpenWorkbook (it requires two parameters, you had an & instead of a ,).

Public Function GetWorkbook(ByVal strFileName As String, Optional ByVal strFilePath As String) As Workbook

    If IsWorkbookOpen(strFileName) Then
        Set GetWorkbook = Workbooks(strFileName)
    Else
        If strFilePath <> "" Then
            Set GetWorkbook = OpenWorkbook(strFilePath, strFileName)
        Else
            PrintErrorMessage "Workbook (" & strFileName & ") is not open, and no filepath was supplied", stopExecution:=True
        End If
    End If

End Function

I like the changes Matt suggested for IsWorkbookOpen so I will not address that function.

I would suggest something similar for OpenWorkbook. I also cleaned up some 1-use variables and made it better indented (plus the * to & typo fix).

Public Function OpenWorkbook(ByVal strFileName As String, strFilePath As String) As Workbook

    On Error GoTo CleanFail
    Dim wbOpenedSuccessfully As Boolean

    Set OpenWorkbook = Workbooks.Open(strFilePath & strFileName)
    wbOpenedSuccessfully = IsWorkbookOpen(strFileName)

CleanExit:
    Exit Function
CleanFail:
    Err.Clear
    PrintErrorMessage "Failed to open workbook """ & strFullFilename & """", stopExecution:=True
    Resume CleanExit

End Function

One thing I might do is add a "does the file even exist?" check here, as that would be better information for the user. Without knowing fully what your error handling does, adding something like:

If Dir(strFilePath & strFileName) = "" Then
    'handle the "file does not exist" error
End If

would provide more information to the user about why the file failed to open.

---

I don't know what PrintErrorMessage does. But consider the situation where the workbook doesn't open, your GetWorkbook method will not return a Workbook object. But all errors are being handled at a lower level, so you won't see any error in the GetWorbook method.

It's possible your PrintErrorMessage will handle this situation (or your code in calling GetWorkbook will verify a valid object is returned), but be mindful you could end up with problems resulting from errors being handled in OpenWorkbook or IsWorkbookOpen.

You might consider passing the err.Description or err.Number messages to your PrintErrorMessage method as those can be useful as well.

Another potential problem I see is if your user passes a filepath without a "\" at the end. For example:

GetWorkbook("myTest.xls", "C:\Temp") 

will fail, because you are doing simple concatenation (you will try to open C:\tempmyTest.xls). Without knowing how users are selecting the folder (are they using a file selection dialog?) it's hard to recommend a robust solution to this, but a simple sanity check could be to verify the file path ends in a \ character and append one if not.

Answer 4

I don't really care for this type of type checking.

strFilePath.TypeName = "String"

I much prefer the typeof operator.

But that doesn't matter, because you're going to listen to Enderland and pass that arg as a String anyway, right?