Error Trapping for dialog box reply

Question

How can I improve upon this code to apply logic based on the user's response to a SaveAs Dialog Box 'do you want to overwrite existing file' question:

Sub SaveFile(fName As String)

cancelSave = False
Retry:
saveAsFileName = Application.GetSaveAsFilename(InitialFileName:=fName, FileFilter:="Excel Files (*.xlsx), *.xlsx")
If saveAsFileName <> False Then
    If Not Dir(saveAsFileName) <> vbNullString Then
        ActiveWorkbook.SaveAs fileName:=saveAsFileName
    Else
        On Error Resume Next
        ActiveWorkbook.SaveAs fileName:=saveAsFileName, FileFormat:=xlWorkbook, ConflictResolution:=xlLocalSessionChanges
        If Err.Number = 1004 Then
            On Error GoTo 0
            GoTo Retry
        Else
            On Error GoTo 0
        End If
    End If
Else
    cancelSave = True
End If

End Sub

Answer 1

It might be a personal preference, but I find indenting the entire procedure body massively helps making a module easier to read - the only thing that's left in column 1 is module-level declarations, procedure signatures, and line labels. Conversely, not indenting the entire procedure body makes it harder to tell which procedure starts/ends where in a module.

Sub SaveFile(fName As String)

The procedure is implicitly Public, which makes it hard to tell whether it's meant to be used in the same module, or if it's meant to be called by code outside the module. If it's meant to be used in the same module, make it Private. If not, make it explicitly Public.

The fName parameter is implicitly passed ByRef, which means if SaveFile reassigns the parameter, the new value will be visible at the call site. If that's intentional, an explicit ByRef modifier should be there. Given the parameter isn't reassigned in the body of the procedure, it should be passed ByVal.

It's not clear how the procedure is used, but it seems cancelSave is declared outside the procedure. If it's in the same module, it should be Private. If it's in another module, then it's Public and you have a global variable... and in all likelihood (assuming SaveFile is called from some BeforeSave event handler) it could be avoided by making the procedure a Function that returns a Boolean:

Public Function SaveFile(ByVal fName As String) As Boolean

I strongly doubt you actually need a global variable for this.

---

The procedure feels wrong, because error-handling logic is intertwined with the actual procedure logic; these On Error and GoTo statements obscure the intent of the code.

Typically, error-handling is done in a dedicated subroutine, and no GoTo statement is needed - you want to run some logic in a loop until the user cancels out; it's much clearer if you use one of VBA's many loop structures instead of implementing that GoTo-loop.

Let's take a moment to consider this alternative implementation:

Public Function SaveFile(Optional ByVal initialPath As String = vbNullString) As Boolean
    On Error GoTo CleanFail
    Dim cancelSaveHandler As Boolean

    Dim completed As Boolean
    While Not completed

        Dim result As Variant
        result = Application.GetSaveAsFileName(InitialFileName:=initialPath, FileFilter:="Excel Files (*.xlsx), *.xlsx")

        If VarType(result) = vbBoolean Then
            'cancelling GetSaveAsFileName dialog returns False:
            cancelSaveHandler = True
            completed = Not result
        'ElseIf Dir(result) = vbNullString Then
        '    'selected file doesn't exist, try saving:
        '    ActiveWorkbook.SaveAs FileName:=result, FileFormat:=xlWorkbook, ConflictResolution:=xlLocalSessionChanges
        Else
            ActiveWorkbook.SaveAs result
        End If

    Wend

    SaveFile = cancelSaveHandler
    Exit Function

CleanFail:
    completed = (MsgBox("An unexpected error has occurred while saving. Try again?", vbYesNo + vbExclamation) = vbNo)
    Resume Next
End Function

Notice:

• The parameter is Optional, because it's not really required; it's also named in a way that doesn't suggest that that's the name we're saving the file with.
• The loop is explicit; there's no GoTo jump.
• The error-handling logic is completely separated from the procedure's actual logic.
• Error handling doesn't leave unexpected errors dangling unhandled, and only ever runs when a runtime error happens.
• No implicit conversion from Variant/String or Variant/Boolean when evaluating whether the GetSaveAsFileName dialog was cancelled.
• No more double-negative when checking if the file exists.
• Checking if the file exists is redundant if we're going to save either way; xlWorkbook is the default format anyway, and what's wrong with prompting for conflict resultion (the default behavior) if/when applicable?
• User knows what's happening and why, at all times.
• We're returning a value to the caller instead of assigning a global variable: there are no side-effects to global state.

In an ideal world, there would also be a VB_Description attribute, to describe what the function does:

Attribute SaveFile.VB_Description = "Prompts user for a non-existing file name and saves the active workbook. Returns True if 'BeforeSave' handler should be cancelled."

You can set this procedure attribute via the Object Browser, by browsing to the function, right-clicking, selecting "Properties" in the context menu, and entering a value in the "Description" box. That description will then be visible in the Object Browser:

Ideally all public members should have a description.

Answer 2

Your logic is horribly unintuitive

The human brain is not good at basic logic. It is absolutely terrible at complex logic. Whenever you have a [condition] statement, you should make it as easy to understand as possible. This means using as few logical operators as possible. Do everything in small steps. Give names to logical outputs like thingExists or isValid.

---

You have 5 seconds to explain what the following conditions are checking for.
GO:

If Not Dir(saveAsFileName) <> vbNullString Then

Versus

Dim fileDoesNotExist as Boolean
fileDoesNotExist = (Dir(saveAsFileName) = vbNullString)

If fileDoesNotExist Then

Notice how you don't have to even explain the second one because it already explains itself.

---

This leads us to the golden rule: Don't make me think. The harder I have to think to deconstruct your logic, the more likely it is that I'll misunderstand it, and if I misunderstand what's going on then I'm going to introduce bugs when I try to do anything with it.

Name things. Make things sound like what they are. Be verbose if it makes the sequence of operations easier to follow.

Also, this:

If saveAsFileName <> False Then

Is confusing, because it is literally the same as writing:

If saveAsFileName = True Then

Which is the same as:

If saveAsFileName Then

---

Tell your user what's going on

    On Error Resume Next
    ActiveWorkbook.SaveAs Filename:=saveAsFileName, FileFormat:=xlWorkbook, ConflictResolution:=xlLocalSessionChanges
    If Err.Number = 1004 Then
        On Error GoTo 0
        GoTo Retry

This is going to cause all sorts of confusion.

I'm a user. I put in a filename and click Save As

What I don't know is that the filename is going to cause a 1004 Error. What do I see?

Nothing.

Let's follow the execution for a second.

1) I click Save As

2) The code passes the If checks.

3) We get to On Error Resume Next

4) The code tries to save the workbook and fails

5) Goto Retry

6) The code re-opens the GetSaveAsFilename dialog box.

As a user, all I see is the result of step 6. I clicked Save As and all that happened was that the box where I put my filename is now blank. WTF? I do the natural thing and try again. I type in my filename, click Save As and ... W.T.F. same thing again. "This useless thing is broken" I think to myself, and now I'm annoyed because I'm on try number 3 and the program isn't responding to me.

Imagine the difference if we just added a little explanation:

    If Err.Number = 1004 Then
        MsgBox "That filename caused an error. Please try again or choose a different filename"
        Goto Retry
    End If

And now the user understands what's happening and knows what they have to do to fix it.

As before, don't make me think. Tell me what I need to know, as cleanly and simply as possible.