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

• Raystafarian: Are you relying on the user to specify the directory and name of the book with all the appropriate characters? Or is there something else that checks that the directory ends with a slash and the file has an excel extension?
• Kaz: These are mostly general-purpose functions for me to use in the rest of my projects. If I wanted the user to select a file, I'd give them a proper file-selection window.

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

• Mathieu Guindon: If {condition} Then Else {do something} doesn't strike me as a go-to solution in terms of readability. Otherwise I agree, one should avoid unnecessary variables whenever possible, but sometimes (often) an intermediate variable is better than nested function calls.
• Dirk Reichel: Just for you and your upvoters, I inserted the IsWorkbookOpen = False... while it is false anyways makes that obsolete to my eyes. ;)
• Mathieu Guindon: Indeed, it doesn't make much sense. How about simply IsWorkbookOpen = Len(Workbooks(strTargetName).Name) > 0?
• Dirk Reichel: Indeed that would be better... however I would just merge this 3 functions... either I need the workbook or not and if I need it, I want to put it into a variable... that said, just 1 Function getWorkbook(...) As Workbook would be enough... wouldn't it?
• Vogel612: Hey there, I edited your post to remove these "EDIT" sections. Since the site automatically keeps a history of revisions including such sections is not necessary, even harmful. Should I have changed your answer to much, please use the rollback feature. Thanks!

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.

• enderland: +1. I go back and forth on using On Error Resume Next for situations like this but for a small function like this would probably do this too for consistency.

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?