best practice for opening workbook excel vba

Question

Is there a best practice for opening an excel workbook? I have this function

Function openWorkbook(workbookpath As String, Optional worksheetname As String)
  If IsMissing(worksheetname) Then worksheetname = "Sheet1"
  'Test workbookpath
  Dim wb As Workbook
  Dim ws As Worksheet
  Dim fso As Scripting.FileSystemObject
  Dim wbname As String
  wbname = fso.GetFileName(workbookpath)
  Dim thisIsSoStupid As Boolean
  thisIsSoStupid = True

  'test if workbook is open
  For Each wb In Workbooks
    If wb.Name = wbname Then
        Set openWorkbook = wb
        thisIsSoStupid = False
        break
    End If
  Next

  If thisIsSoStupid And Dir(workbookpath) <> 0 Then
   openWorkbook = Workbooks.Open(workbookpath)

  Else
    openWorkbook = False
  End If
End Function

I'd like to do something like g = openWorkBook(path)||false which is a javascript shortcut or $g = fopen($path, 'w') or die('bugger'); but of course, if it can't find the file, it will return false and I can't evaluate a false if my code is expecting an object. Do I have to on error resume and then catch the error? I hate the stupid error crap. It's an invitation to spaghetti code

Answer 1

Your function doesn't declare a return type, so it's implicitly Variant.

You're returning a Boolean False when it fails, and a Workbook object reference when it succeeds. This makes the client code much harder to write than necessary.

Instead, make the function return a Workbook reference, and return Nothing when it fails.

Also, if you supply the default value for your optional parameter in the signature, then your IsMissing check becomes redundant.

Now, I'm not sure why you would need a worksheet name when you're concerned about opening a workbook - especially since, well, you're not doing anything with that worksheetname parameter. Pre-exception error handling in the language isn't the only thing responsible for spaghetti code.

Here's a more sane version:

Public Function OpenWorkbook(ByVal path As String) As Workbook
    On Error GoTo CleanFail

    Dim result As Workbook        
    If Not IsWorkbookOpen(path, result) Then
        Set result = Application.Workbooks.Open(path)
    End If

CleanExit:
    Set OpenWorkbook = result
    Exit Function
CleanFail:
    Set result = Nothing
    Resume CleanExit
End Function

Public Function IsWorkbookOpen(ByVal path As String, Optional ByRef outWorkbook As Workbook = Nothing) As Boolean
    Dim book As Workbook
    For Each book In Application.Workbooks
        If book.FullName = path Then
            IsWorkbookOpen = True
            Set outWorkbook = book
            Exit For
        End If
    Next
End Function

Notice:

• Explicit Public access modifier
• Explicit ByVal modifier; your code implicitly passes parameters ByRef but doesn't need to.
• Explicit and unambiguous return type
• PascalCase names for public members, to conform with the naming style of every single existing VBA API
• Separated concerns: verifying whether a workbook is open, vs. opening a workbook
• Explicit default value for optional parameters
• Explicit ByRef modifier where appropriate
• out prefix to denote an "out" parameter
• Meaningful identifiers everywhere

These aren't "THE" best-practices, they're just how I write VBA. I'm pretty sure having thisIsSoStupid as a variable name in your code is universally seen as unprofessional code though.

Ranting on the "stupid error crap" is fine (I'd much rather deal with exceptions too), but not when your code has unused parameters, ambiguous return types and poor naming.