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. The first version is known as the Arrow anti-pattern, and “flat” code is in general preferable; there is a good discussion on the topic here:
   http://www.codinghorror.com/blog/2006/01/flattening-arrow-code.html
   Cheers,

   Mathias

3. 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?

4. 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.

5. 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?

6. 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.

7. 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

8. Neato. Tbumbs up. Season’s greetings

9. Pingback: Daily Dose of Excel » Blog Archive » Finding Almost This Friday

10. 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


11. 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

12. 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.

13. 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.

---

Posting code? Use <pre> tags for VBA and <code> tags for inline.