Procedure Flow and Raising Errors

I have a pretty simple procedure that I run a few times every other week. Someone emails their timesheet to me, I open the timesheet in Excel, and I want a quick way to save it to the proper directory. GMail stores the timesheet in my Temp directory and navigating through the file structure is a pain. I wrote this procedure and it includes the following conditions:

  • Is there an active workbook
  • Is that workbook a timesheet
  • Has the timesheet already been saved
  • Does the path exist

That results in four levels of nested ‘If’ statements. I like nested statements. I think it makes the code very readable and easy to follow.

Public Sub SaveDownloadedTimesheet()

Dim wb As Workbook
Dim dtThisFriday As Date
Dim sNewPath As String
Dim sNewName As String

Const sPAYROLLPATH As String = "\\Server\Share\Accounting\Payroll\Processing\"
Const sCOPY As String = "Copy of "

On Error Resume Next
Set wb = ActiveWorkbook
On Error GoTo 0

If Not wb Is Nothing Then
If IsTimesheet(wb) Then
If IsTemp(wb) Then
dtThisFriday = ThisFriday
sNewPath = sPAYROLLPATH & Format(dtThisFriday, "mmdd") & "\"

If Len(Dir(sNewPath, vbDirectory)) > 0 Then
sNewName = Replace(wb.Name, sCOPY, "", 1, 1)
wb.SaveAs sNewPath & sNewName
wb.Close False
End If
End If
End If
End If

End Sub

The code works great except when it doesn’t. When there’s a problem, I don’t get any kind of message telling which one of my conditionals failed. It’s usually that the path doesn’t exist, but today I was absolutely freaking positive that the path existed and it still wasn’t saving. Of course I was wrong, but that’s not the point.

One solution is to add an Else clause to each If statement with a message box. There’s nothing wrong with that solution, but I don’t find it aesthetically pleasing. Did I just say “aesthetically pleasing”? My goodness. It adds eight lines of code and, to me, ruins the flow of the program.

An alternative is to raise errors. That might look like this:

Public Sub SaveDownloadedTimesheet()

Dim wb As Workbook
Dim dtThisFriday As Date
Dim sNewPath As String
Dim sNewName As String

Const sPAYROLLPATH As String = "\\Server\Share\Accounting\Payroll\Processing\"
Const sCOPY As String = "Copy of "

On Error GoTo ErrHandler

On Error Resume Next
Set wb = ActiveWorkbook
On Error GoTo ErrHandler

If wb Is Nothing Then Err.Raise 9999, , "No active workbook"
If Not IsTimesheet(wb) Then Err.Raise 9999, , "Active workbook is not a timesheet"
If Not IsTemp(wb) Then Err.Raise 9999, , "Timesheet already saved"

dtThisFriday = ThisFriday
sNewPath = sPAYROLLPATH & Format(dtThisFriday, "mmdd") & "\"

If Len(Dir(sNewPath, vbDirectory)) = 0 Then Err.Raise 9999, , "Can't find folder for " & Format(dtThisFriday, "mmdd")

sNewName = Replace(wb.Name, sCOPY, "", 1, 1)
wb.SaveAs sNewPath & sNewName
wb.Close False

ErrExit:
Exit Sub

ErrHandler:
MsgBox Err.Description
Stop
Resume

End Sub

I don’t like that as much as nicely nested Ifs, but I like it better than a bunch of Else clauses. Most of my conditions are nicely grouped, so it’s not difficult to see what I’m checking. And it tells me the problem, so that’s nice. Which do you prefer? Do you use a different method?

Posted in VBA

13 thoughts on “Procedure Flow and Raising Errors

  1. I like the second, un-nested one better. I’d like it even more betterer if the checks were pushed down to a subsidiary checking function, so the error handling boils down to (hopefully) a single line in SaveDownloadedTimesheet(), which becomes all about what happens when it works. I might make the path construction into a separate function too: I like my routines *small*…

  2. Merry Christmas Dick Kusleika and to all your contributors/readers.

    VBA (programming) is a hobby of mine, a different level than those of you that do this for a living.

    I’ll answer your question & ask a question.

    answer: I like the last example better. It is easier to see what is being checked, even though I might do a lot of if/then/elses to get to this (final) point.

    question: I see you have sCopy as a constant, but err.discriptions as not. If you know, what is the difference in making one a constant and the other not?

    In an application, which one would save time/effort in writing, compiling, debugging, memory allocation, and execution speed? What would be the difference between sub/function constants v/ global constants v/ string like you did?

  3. I think the second version is much better.

    As you note, if one of your checks fails, it’s easy to figure out which one. More important, you don’t have to handle the exceptions in the same routine that raises them. Somewhere higher up in the call stack might well be the better place to deal with the problem. (Maybe not in this specific routine if what you want is a message box, but in general…)

    I also think it’s easier to read because you clearly separate out your error-detecting logic from your timesheet-saving logic, but that’s just a matter of differing tastes.

  4. I quite often use the 2nd layout in much the same way you’ve provided – up front validation, then core function.

    A question though, did you mean to write Resume, or Resume ErrExit?

  5. Wow, all good comments. Mike, I like moving the first three conditions into a separate procedure, but what about the fourth? I’ll try it out.

    ddDaniels – In general, I want to have no strings or numbers other than zero and one in my code – they are always made constants. However, I don’t actually convert them to constants until I’m satisfied the code works. And I say “in general” because sometimes I just don’t do it (but I should).

    Rob – I used Resume in this case because I’m the only one who uses this code and I wanted to get kicked into the procedure and debug. But Resume ErrExit would be proper for production code.

  6. I tried Mike’s suggestion of pulling the conditional code out and here’s what I got. I also used some advice I’ve gotten from Bob Phillips about moving On Error to it’s own procedure, so I moved getting the active workbook out. Finally I cleaned up the code into using constants.

    Private Const msFLDRFMT As String = "mmdd"
    Private Const mlHANDLED_ERROR As Long = 9999

    Public Sub SaveDownloadedTimesheet()

    Dim wb As Workbook
    Dim dtThisFriday As Date
    Dim sNewPath As String
    Dim sNewName As String
    Dim sErrorMsg As String

    Const sPAYROLLPATH As String = "C:\Accounting\Payroll\Processing\"
    Const sCOPY As String = "Copy of "

    On Error GoTo ErrHandler

    Set wb = GetActiveWorkbook

    dtThisFriday = ThisFriday
    sNewPath = sPAYROLLPATH & Format(dtThisFriday, msFLDRFMT) & Application.PathSeparator

    If Not IsSaveableTimesheet(wb, dtThisFriday, sNewPath, sErrorMsg) Then Err.Raise mlHANDLED_ERROR, , sErrorMsg

    sNewName = Replace(wb.Name, sCOPY, Space(0), 1, 1)
    wb.SaveAs sNewPath & sNewName
    wb.Close False

    ErrExit:
    Exit Sub

    ErrHandler:
    MsgBox Err.Description
    'Stop
    Resume ErrExit

    End Sub

    Public Function IsSaveableTimesheet(wb As Workbook, dtThisFriday As Date, sNewPath As String, ByRef sErrorMsg As String) As Boolean

    Dim bReturn As Boolean

    Const sERR_NOACTIVE As String = "No active workbook"
    Const sERR_NOTTIME As String = "Active workbook is not a timesheet"
    Const sERR_ALREADYSAVED As String = "Timesheet already saved"
    Const sERR_NOFOLDER As String = "Can't find folder for "

    On Error GoTo ErrHandler
    bReturn = True

    If wb Is Nothing Then Err.Raise mlHANDLED_ERROR, , sERR_NOACTIVE
    If Not IsTimesheet(wb) Then Err.Raise mlHANDLED_ERROR, , sERR_NOTTIME
    If Not IsTemp(wb) Then Err.Raise mlHANDLED_ERROR, , sERR_ALREADYSAVED
    If Len(Dir(sNewPath, vbDirectory)) = 0 Then Err.Raise mlHANDLED_ERROR, , sERR_NOFOLDER & Format(dtThisFriday, msFLDRFMT)

    ErrExit:
    IsSaveableTimesheet = bReturn
    Exit Function

    ErrHandler:
    bReturn = False
    sErrorMsg = Err.Description
    Resume ErrExit

    End Function

    Public Function GetActiveWorkbook() As Workbook

    On Error Resume Next
    Set GetActiveWorkbook = ActiveWorkbook

    End Function

  7. All this is too much song and dance. IMO, the error handling has gone way past the “point of diminishing returns.”

    I don’t like “raising errors” in code that is completely within my control. It’s an useful technique when used correctly with code shared across groups or with customers, where I don’t know who, if anyone, will handle the error.

    We’ve discussed the below before but I guess it’s worth repeating. {grin}

    So, option 1, which doesn’t strictly follow the rules of structured programming:

    Sub Demo1()
    Dim ErrMsg As String
    If wb Is Nothing Then ErrMsg = "No active workbook": GoTo ErrXIT
    If Not IsTimesheet(wb) Then ErrMsg = "Active workbook is not a timesheet": GoTo ErrXIT
    If Not IsTemp(wb) Then ErrMsg = "Timesheet already saved": GoTo ErrXIT
    'do regular stuff
    Exit Sub
    ErrXIT:
    'handle error
    End Sub

    Option 2, which adheres to the rules of structured programming and contains no GoTos:

    Sub Demo2()
    Dim ErrMsg As String
    If wb Is Nothing Then
    ErrMsg = "No active workbook"
    ElseIf Not IsTimesheet(wb) Then
    ErrMsg = "Active workbook is not a timesheet"
    ElseIf Not IsTemp(wb) Then
    ErrMsg = "Timesheet already saved"
    Else
    'do regular stuff
    End If
    If ErrMsg <> "" Then
    'handle error
    End If
    End Sub

  8. I typically keep error handling for unexpected errors (where my standard error handling logs the error as well as sending a message to the user). However, I do like the flow of option 2 – because it checks for conditions before continuing and the code does not get too nested.

    So typically I use option 2 but with the msgbox rather than err.raise followed by a Goto CleanUp which is a label near the exit of the function to handle all cleanup activities if there are any.

    I am sure I would not bury the condition checking in a separate routine – just too pretty for me – and I would not create constants of error message text – that is makes it harder to understand the code, rather than easier – my personal preferences clearly.

    –Charlie

  9. I also like #2. I agree with Tushar (and Charlie) about not raising errors for anticipated problems like this, and use something like his #1 option. I like to limit error raising to unexpected errors.

    I’ve got one project where I pushed these anticipated errors down to a sub-function because there were about seven of them. I’m not sure I like it, I had to pass a few variables, and the debugging actually seemed more complicated than if all the anticipated errors were checked at the top of the routine.

  10. Why would you have four nested levels of ‘If… Then’ branching when you’re running a sequential (NOT branching!) data validation check the parameters?

    Your logic is this ‘branch’:

    IF [All params are validated] THEN
    ‘ Proceed
    ENDIF

    Which follows on from this sequence of ‘drop-through’ binary tests:

    Check parameter A: IF valid THEN proceed ELSE Raise error A and Exit
    Check Parameter B: IF valid THEN proceed ELSE Raise error B and Exit
    Check Parameter C: IF valid THEN proceed ELSE Raise error C and Exit
    Check Parameter D: IF valid THEN proceed ELSE Raise error D and Exit

    There’s no combinatorial logic ‘IF A, C and D are valid, but B failed the test…’ etc that differs from detecting the first failed validation. So there’s no need to implement a nested structure that allows the implied ELSE clauses to handle some (but not all!) of the possible cases of partial validation failure.

    The structure that gives the closest match to the logic, and avoids GOTO (or implicit GOTO statements that use the error-handler) is Tushar Mehta’s If… ElseIf cascade.

    This ElseIF cascade structure offers at-a-glance comprehension, and a single, simple insertion point for additional validation logic.

    I have to admit that I tend to structure this kind of code as a sequential validation block with ‘GOTO’ (implemented as Exit Sub, or ‘Exit-using-the-error-handler’, but it’s still a GOTO), then a clearly-marked ‘processing’ block. Yes, there are good reasons why this is considered harmful.

    But the purists’ choice of nested IF… THEN blocks is arguably worse. It’s not that nested IFs would be difficult to read, in and of themselves: we can all read code. The problem is that this implements a branching structure that doesn’t actually reflect the simple linear flow of logic. This post is, of course, a trivial example – and all the better for it – but all of the nontrivial examples of unneccesary complexity that we encounter in other peoples’ code (and, sometimes, our own) contain one or more of these confusing cases where the visible implementation diverges from the underlying logic.

    The short version: slip in an ‘Else’ clause in the nested structure, and see if you can still read the logic at a glance.

Leave a Reply

Your email address will not be published. Required fields are marked *