# Get Workbook Method(s)

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

• 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? – Raystafarian Dec 16 '15 at 15:40
• 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. – Kaz Dec 16 '15 at 15:52

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

• 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. – Mathieu Guindon Dec 16 '15 at 16:05
• Just for you and your upvoters, I inserted the IsWorkbookOpen = False... while it is false anyways makes that obsolete to my eyes. ;) – Dirk Reichel Dec 16 '15 at 16:50
• Indeed, it doesn't make much sense. How about simply IsWorkbookOpen = Len(Workbooks(strTargetName).Name) > 0? – Mathieu Guindon Dec 16 '15 at 16:53
• 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? – Dirk Reichel Dec 16 '15 at 17:50
• 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! – Vogel612 Dec 17 '15 at 13:32

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.

• +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. – enderland Dec 16 '15 at 16:45
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


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.

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?