Error Trapping for dialog box reply 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 &lt;&gt; False Then
    If Not Dir(saveAsFileName) &lt;&gt; 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
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.