VBA Confessions

I’m guilty and I’d like to come clean. I’m probably guilty of a lot of things, but I only wish to confess two transgressions. I’ve been working with some code lately, some mine, some not, and it made me appreciate proper coding techniques.

I’m generally a believer in the one-exit-point rule. But I admit I don’t follow it religiously. One-exit-point means that every procedure has only one way out. In VBA, there are generally two ways to get out of a procedure, End Sub and Exit Sub. There’s also End, but I’ve never had cause to use it and I never will. There is one particular code construct I like that violates this rule.

Public Sub SomeSub()
   
    If SomeCondition Then Exit Sub
    If SomeOtherCondition Then Exit Sub
    If SomeThirdCondition Then Exit Sub
   
    DoRealWorkHere
   
End Sub

I like this construct because it puts all my failure modes at the top of the sub. They’re easy to see and easy to edit. Recently, I had to run another procedure at the end of this one, regardless of whether it failed. Without those Exit Subs, I could have called the procedure right before the End Sub and been done with it. Instead, I had to code it like this.

Public Sub SomeSub()
   
    If SomeCondition Then
        LoggingProc
        Exit Sub
    End If
   
    If SomeOtherCondition Then
        LoggingProc
        Exit Sub
    End If
   
    If SomeThirdCondition Then
        LoggingProc
        Exit Sub
    End If
   
    DoRealWorkHere
   
    LoggingProc
   
End Sub

I can’t say I’ve run into this a lot, but it really drove home the value of one exit point. Had I been more rigorous, I would have coded it like this

Public Sub SomeSub()
   
    On Error GoTo ErrorHandler
   
    If SomeCondition Then Err.Raise glERRQUIET
    If SomeOtherCondition Then Err.Raise glERRQUIET
    If SomeThirdCondition Then Err.Raise glERRQUIET
   
    DoRealWorkHere
   
ErrorExit:
    Exit Sub

ErrorHandler:
    If Err.Number = glERRQUIET Then
        Resume ErrorExit
    Else
        Stop
        Resume
    End If
   
End Sub

Now I have one exit point; the Exit Sub in the ErrorExit label. Any time I want to exit a sub quietly, I can raise an error with my global quiet error constant. I still get all my failure conditions at the top and nicely grouped. When I need to incorporate LoggingProc, I only have to call it once.

My next sin has to do with entry point procedures. Entry point procedures are procedures that the user can cause to run, as opposed to procedures that are called form other procedures. In the context of an application, that means they can be called via a control placed on a spreadsheet; a menu, toolbar, or ribbon control; or by firing an event (like the Worksheet_Change event). In the vast majority of my projects, I have only three places that I have to look for entry point procedures, and it’s usually only two.

  1. The MEntryPoints module
  2. The CAppEvents custom class module
  3. The ThisWorkbook and sheet class modules

I realize that the third one isn’t one place, but you get my meaning. If my code is an add-in, all of the event code is in CAppEvents and all of the non-event entry points are in MEntryPoints. Except when I don’t apply the level of rigor that I should. In some cases, I get lazy and put an entry point procedure in a different standard module.

What’s the harm? Here’s an example: Due to some changes, I now need to verify that a particular workbook is open before my code can run. I have to identify every way the code can start (entry points) and check for the existence of the workbook. When I know where they all are, it’s relatively easy. When I don’t, it’s not.

Those are my sins. I shall endeavor to improve. Where do you cheat?

Posted in Uncategorized

18 thoughts on “VBA Confessions

  1. I realise this may be considered heretical, but I wonder if you might consider a Go To instead of that Err.Raise thing, which is IMHO just a Go To in disguise (because it’s not really raising Errors at all)?

    So:

    Public Sub SomeSub()
    If SomeCondition Then Go To Finally
    If SomeOtherCondition Then Go To Finally
    If SomeThirdCondition Then Go To Finally

    DoRealWorkHere

    Finally:
    LoggingProc
    End Sub

    looks better to me. Alternatively, consider wrapping the three Some*Conditions into a separate function, which would get you to something like:

    Public Sub SomeSub()
    If SomeCombinationOfConditionsSatisfied Then
    DoRealWorkHere
    End If
    LoggingProc
    End Sub

    … which I like better.

  2. I use a similar technique to Mike’s above, assumed from my practice with VB.Net,, and is try to mimic a Try Catch Finally.

    Something like

    Public Sub Test()

    On Error Goto catch
    ‘Code here
    Goto finally
    catch:
    ‘Error handler here
    finally:
    ‘Clean up stuff here
    End Sub

  3. I use your code construct #1 to handle semantic errors, for example checking the length of a String parameter to make sure the calling procedure is passing a valid parameter. The compiler wouldn’t be able to catch this sort of error. I’d rather do it that way than letting the error occur.

    Normally I use an “On Error GoTo ErrorHandler” like your code construct #3.

  4. Sub someSUB
    If Not SomeCondition1 then
    If Not SomeCondition2 then
    If Not SomeCondition3 then
    RealWork
    End if
    End if
    End if
    End Sub

  5. I use the technique that Mike indicates – probably picked up from PED – those are not always error checks, just conditions where you know that the real work does not need to be done.

    I personally like Goto CleanUp rather than Finally, but that is just what looks good to me. The first thing that is in CleanUp is On Error Resume Next, so I can do all sorts of cleanup without having to worry about causing errors (ie set obj = nothing – when obj had not been set yet)

    –Charlie

  6. @Dick, I’m with you, raise a pseudo-error. May be no better than GoTo, but I like it. And as you know from an earlier blog, anything that might fail and thus entails a OnError Resume Next I like to delegate to a separate function.

    @Charlie, if the object could be Nothing, test it and destroy it if there – better to avoid the OnError Resume Next where possible.

  7. “Heretic” of Toronto here.
    Hi.
    I think that “On Error” should be restricted ONLY to library functions, and ought not appear in any application code.
    I agree with your thinking on single-exit-points however.
    And this leads me to believe that discussions on a procedure such as your example probably indicate a design flaw, and most likely the solution is to develop finer-detailed procedures.
    (1) On Error: The original VBA had no FSO, so we all developed a blnFileExists function that trapped, with On Error, the statement FileLen(strFileName)=FileLen(strFileName). The function blnFileExists is essentially a patch for VBA, and as such properly belongs in a library of VBA procedures Utils.dot or similar; app;lication programmers ought not to include this sort of code in an application.
    (2) Single-exit: Why not something generally understood by 95% of programmers alive today?
    If SomeCondition Then
    Else ‘ Some descriptive comment here explaining the decision or filter
    If SomeOtherCondition Then
    Else ‘ Some descriptive comment here explaining the decision or filter
    If SomeThirdCondition Then
    Else ‘ Some descriptive comment here explaining the decision or filter
    DoRealWorkHere
    EndIf
    EndIf
    EndIf
    (3) Writing procedures with no more than two arguments
    Oh, I see we haven’t got to that yet (grin) so I’ll withhold my comments.

  8. That AntiPattern page is probably one of the worst reference pages I have seen in many a day. The guy seems intent on writing those pages as he thinks we should all code, very little on a page, and anything of interest is on another referenced page. So to actually follow a discussion, you end up visiting 30 or 40 pages … pah, garbage! He even has a link to a page called ‘OneReturnPerFunction’ which immediately links to yet another page, why not link directly to the where the ‘information’ is?

  9. Since all your ‘Ifs’ lead to the same point (LoggingProc), why not a single ‘If’ followed by “Ors”?

  10. In the good old days when I learnt C, my instructor drilled in to us “Never use GoTO” other than for err handling…the habit stayed.

  11. Sorry to be a pain, but

    If c1 Then Exit Sub
    If c2 Then Exit Sub
    If c3 Then Exit Sub
    DoRealWork

    could be rewritten as

    If Not (c1 Or c2 Or c3) Then
    DoRealWork
    Else
    OptionalErrorLogging
    End If

    The latter may be highly inefficient and/or error prone if it’s unnecessary or even dangerous to check c2 and c3 when c1 is true. Since MSFT BASIC dialects lack short-circuit boolean evaluation, it’s often necessary to use the arrowhead antipattern sam mentioned.

    MSFT BASIC has poor exception handling syntax too. Java’s try/catch is a lot better for runtime exception handling, but that still leaves the matter of a single exit point if no code past the catch block should run.

    Better to check conditions in the calling procedure when possible. Only when the caller confirms no exceptions would it call DoRealWork. However, that leaves system calls encapsulated in procedures which may or may not throw exceptions. If the system call and following DoRealWork code is only rarely needed, it’s so much better to bundle system call, standard follow-up code and exception handling in a single place and accept the arguable ugliness of nontrivial exception handling than to deal with the syntactic contortions needed for aesthetically pure code.

    VBA is a poor language to use for anything that requires a fair amount of exception handling. The only reason so many put up with VBA is because there’s seldom an alternative. But if you’re going to use a poorly designed language like VBA, don’t expect to be able to adhere to coding rules developed for better designed languages.

  12. @Bob Phillips: yes, I don’t like the style of that website either, but sadly that’s where all the popular links point. However, it’s an aside: don’t allow it to distract you and @sam from the point I am making :) The arrowhead is widely acknowledged as an antipattern.

  13. I also try to have one exit point per function.
    Due to VBA not having short circuits, I do it this way:

        Dim bln As Boolean

        bln = Not c1
        If bln Then bln = Not c2
        If bln Then bln = Not c3
        If bln Then
            DoRealWork
        Else
            OptionalErrorLogging
        End If

    I’d probably go a step further and have c1, c2, c3 return True to continue, rather than True to error.

  14. @Bob Phillips: yes, I recall you were always looking out across Poole Harbour to the Purbecks, rather than getting on with some proper work… and there’s nothing wrong with that ;)

Leave a Reply

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