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.
|
1 2 3 4 5 6 7 8 9 |
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.
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 |
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
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 |
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.
- The MEntryPoints module
- The CAppEvents custom class module
- 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?
I use On Error Resume Next waaaaay too often.
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.
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
On Error Goto catch
‘Code here
Goto finally
catch:
‘Error handler here
finally:
‘Clean up stuff here
End Sub
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.
Sub someSUB
If Not SomeCondition1 then
If Not SomeCondition2 then
If Not SomeCondition3 then
RealWork
End if
End if
End if
End Sub
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
@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.
“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.
I tend to use the first method, which usually means there is nothign to cleanup, because I do the tests before any other “work” is done.
@sam: Yours is an example of the “arrowhead anti-pattern” (http://c2.com/cgi/wiki?ArrowAntiPattern), did you intend to make a confession…?
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?
Since all your ‘Ifs’ lead to the same point (LoggingProc), why not a single ‘If’ followed by “Ors”?
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.
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.
@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.
I also try to have one exit point per function.
Due to VBA not having short circuits, I do it this way:
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.
@Jamie, I struggle to stop anything distracting me :)
@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 ;)