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
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
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
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
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
SaveFile = cancelSaveHandler
completed = (MsgBox("An unexpected error has occurred while saving. Try again?", vbYesNo + vbExclamation) = vbNo)
- 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
- 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/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.