Inline error checking in VBA

Question

I asked a question elsewhere on Stack Exchange and was given an answer by multiple people that checking for errors in-line was not a good practice. I have been using an

on error resume next
' do something
on error goto 0

block structure for years as kind of an improvised try-catch construct for VBA.

Here is a simple example of some actual code:

I need to check if an object has been passed before I attempt to access the properties of the object. Rather than having the same Error handling label in each subroutine that accesses that object, I've opted to do my error check in a function that returns a Boolean value telling me whether or not the object has been initialized.

I want to know if this is acceptable practice, or if there is a better way this should be handled.

NOTES:

This is NOT the complete code. I just took the relevant props/methods for the question and added them below. The main program allows data sharing, updating, and communication between three different systems. This example is taken from a class object which bridges communication between a corporate intranet website (through ASP) and a Solidworks model object, whose information is made available using the Class_MyModel object.

Option Explicit
 'Declare module level constants
 Private Const errNoMyModel As Long = -999
 'Declare module level variables
 Private pMyMod As Class_MyModel
'PROPERTY that holds an instance of custom class MyModel which provides all the model info from solidworks
Public Property Get MyMODEL() As Class_MyModel
 Set MyMODEL = pMyMod
End Property
Public Property Let MyMODEL(object As Class_MyModel)
 Set pMyMod = object
End Property
'METHOD which can be called to submit a drawing
Public Sub SubmitDrawing()
 DoSubmittal
End Sub
Private Sub DoSubmittal()
'This procedure updates the intranet site with info from the mymodel object _
 it will create a new record if none exists for the given rev, or update the _
 record if one already exists
 'Check first that the mymodel object property has been set
 If Not MyModelExists Then Exit Sub 'Nothing can be done if we don't have a mymodel object to work with
'Declare variables
 Dim strURL As String
 Dim blnExists As Boolean
'Call the procedure that deals with existing records.
 blnExists = HandleExisting(False)
 'The procedure above returns a true value if a record already existed for the current revision, so if it returns false, we need to add a new record for the current revision
 If Not blnExists Then
 'Construct the URL that will add a new record for the current revision
 strURL = GetURL(aNew)
 'Call the procedure to execute the URL. Print the return text to the debug window so it can be reviewed if necessary.
 Debug.Print MyMODEL.PARTNO & " " & MyMODEL.REVISION & " CREATE NEW RECORD ATTEMPT RETURNED : " & vbCrLf & DoASP(strURL, False) 'Calls the procedure to exectue the URL and prints the ASP return message to the debug window.
 End If
'Activate the current the record for the current revision
 HandleExisting (True)
End Sub
Private Function MyModelExists() As Boolean
'This function simply checks that the calling procedure has successfully passed a Class_MyModel object to work with
'Declare variables
 Dim tempModel As Class_MyModel
'Attempt to set an object to reference the mymodel object instance
 On Error Resume Next
 Err.Clear
 Set tempModel = MyMODEL
 On Error GoTo 0
 'Check if the attempt was successful, if not, then no mymodel object exists. _
 Return the result to the calling procedure
 If tempModel Is Nothing Then
 MyModelExists = False
 ErrorMsg (errNoMyModel)
 Else
 MyModelExists = True
 End If
 If Err.Number > 0 Then ErrorMsg
'Cleanup objects before ending the procedure because don't entirely trust VBA's garbage collection
 Set tempModel = Nothing
End Function
Private Sub ErrorMsg(Optional ByRef ErrNum As Long)
'This is a simple error message handling procedure
'Choose what do do based on the error that occurred
 Select Case ErrNum
 Case errNoMyModel
 'This is a constant delcared at the module level that is used to identify an error where the mymodel property has not been set before attempting to call the procedure to update the entry
 MsgBox "In order to proceed, a MyModel object containing a Solidworks model must be passed to this object. " & _
 "Please check that you have selected a valid Solidworks object (.sldprt, .slddrw, .sldasm). " & _
 "If you continue to get this message when a valid object is selected, please contact the software developer " & _
 "to resolve this problem.", vbOKOnly, "ERROR: Object not assigned"
 Case Else
 'For all other erros, just inform the user of what happened
 MsgBox "ERROR! " & vbCrLf & "Error type: " & Err.DESCRIPTION & vbCrLf & "Error number: " & Err.Number & vbCrLf & "Error source: " & Err.Source, vbOKOnly, "ERROR"
 End Select
End Sub

• Mathieu Guindon: Welcome to CR! Feel free to format the code block so as to better reflect your actual indentation, it makes it easier to read! =)
• CBRF23: I made some edits and it goofed the spacing all up! Is there a way to control it? Whenever I "ctrl-k" on the whole block, it adds more indents to whatever was existing and leaves whatever was new at the first level... I just copied it back to a VBA module, adjusted the indents, and then and re-pasted the whole thing back in.
• Malachi: it's tricky, there are a couple of posts in meta about formatting the code blocks for here. Check out this Meta Answer

Answer 1

Nitpicks

Private Const errNoMyModel As Long = -999

I like that you're declaring a constant for custom errors. Best practice would be to add the built-in vbObjectError constant to your own custom error codes - and for better maintainability, it's often best to define these constants in an Enum:

Private Enum MyModelError
 ModelNotSet = vbObjectError + 999
 ServerNotFound
 InvalidUrl
 OtherCustomError
End Enum

The name given to errNoMyModel looks like a private field or local variable. Constants are usually clearer in YELLCASE... but I read that identifier as "error number for MyModel", which means absolutely nothing. Contrast to MyModelError.ModelNotSet, which tells you just by its name, that the model isn't set on MyModel.

Speaking of MyMODEL:

Public Property Let MyMODEL(object As Class_MyModel)
 Set pMyMod = object
End Property

This should be a Property Set accessor; Property Let works better for value types. Besides client code that does this:

model.MyMODEL = instance

Looks very confusing, given that instance is an Class_MyModel instance. But without a Property Set accessor, client code can't even do this:

Set model.MyMODEL = instance

...which would be the correct and expected way to assign an object reference.

I don't understand the need for this method:

Public Sub SubmitDrawing()
 DoSubmittal
End Sub

Why not make DoSubmittal a Public member, and simply call it Submit?

Note, vbCrLf is Windows-specific. Better use vbNewLine instead.

And this...

'Cleanup objects before ending the procedure because don't entirely trust VBA's garbage collection
Set tempModel = Nothing

VBA doesn't do garbage collection, it does reference counting: that line is utterly useless, since tempModel is locally declared - its reference is destroyed as soon as the procedure exits.

By the way, nulling a reference in a garbage-collected language (like VB.NET, or C#) would not force garbage collection.

---

Reference Check

If Not MyModelExists Then Exit Sub

That's very good. What's less good, is what's under the covers here:

Private Function MyModelExists() As Boolean
'This function simply checks that the calling procedure has successfully passed a Class_MyModel object to work with

The problem is that...

'Attempt to set an object to reference the mymodel object instance
On Error Resume Next
Err.Clear
Set tempModel = MyMODEL
On Error GoTo 0

Assigning a null reference (Nothing) isn't illegal, so this assignment will never blow up; you don't need to expect an error here. In fact, you don't even need this tempModel - and this is overkill:

 If tempModel Is Nothing Then
 MyModelExists = False
 ErrorMsg (errNoMyModel)
 Else
 MyModelExists = True
 End If

You could just do this instead:

MyModelExists = (Not MyMODEL Is Nothing)

...and then you don't even need a MyModelExists function, you could just inline that simple check.

---

Error Handling

What you're trying to do here, is gracefully handle the runtime error 91 that would occur if DoSubmittal were to execute without MyMODEL being set.

As per your post, we're not seeing the whole picture. That's sad, because based on what I'm seeing, this whole "ensure MyMODEL is set" spaghetti looks futile, since MyMODEL is really a dependency of the DoSubmittal method, and should be passed as a parameter.

But let's say it has to be an instance field because other members need to access it later (or earlier... whatever).

Here's how I'd handle this - I would have a procedure responsible solely for assigning the member values; this procedure would need to handle the case where MyMODEL is not set:

Private Sub AssignMemberValues(ByVal result As WhateverThisIs)
 On Error GoTo CleanFail
 MyMODEL.PARTNO = result.PartNumber
 '...
 Exit Sub
CleanFail:
 If Err.Number = 91 Then 'object variable not set
 'raise meaningful error with custom error message:
 Err.Raise MyModelError.ModelNotSet, TypeName(Me), ERR_MODEL_NOT_SET
 Else
 Err.Raise Err.Number ' rethrow if we don't know how to handle
 End If
End Sub

The calling code (perhaps the DoSubmittal procedure) can then handle all errors with a simple message box, because any error that could be raised in the procedures called by this one would contain a specific and meaningful description:

Public Sub DoSubmittal()
 On Error GoTo CleanFail
 '...
 result = GetValues 'may raise ServerNotFound or InvalidUrl errors
 AssignMemberValues result 'may raise MyModelError.ModelNotSet error
 '...
CleanExit:
 'clean-up code goes here
 Exit Sub
CleanFail:
 MsgBox Err.Description
 Resume CleanExit
End Sub

The key here, is to avoid God-like methods that do everything that ever needs to happen: by splitting the work into specialized methods that do one thing (and ideally, do it well), you limit the number of runtime errors you need to handle.

Bottom line, On Error Resume Next is hardly ever an option for clean code.

• CBRF23: Love this: MyModelExists = (Not MyMODEL Is Nothing)
• CBRF23: I do like the idea of having an Assinger routine to set all the properties and can handle all errors there. That makes sense. I usually use YELLCASE to distinguish class properties. I've started adding AvarName for arguments, MvarName for module level variables, and CvarName for constants. I wish I could post the whole project, but there's a lot going on with connecting to internal databases and systems that I can't expose. Maybe if I get some down time (not often) I can go through and "dummy" them out.
• Mathieu Guindon: @CBRF23 I gotta admit, I deeply hate Hungarian notation. Public members are PascalCase, locals and parameters are camelCase, constants are YELLCASE - as for private fields, I have a trick: I only ever have 1 private field called this: I define a Private Type TMyClass and that's where I put the fields in, so there's never a name clash, and this.Something is clearly an instance field ;-)
• CBRF23: Oooh. I like that! this.Something very nice!
• CBRF23: So I just wanted to add to this discussion - most of my work has been done in VBA, and I just started playing in visual studio express. In VSE the intellitype is so much better that there really is absolutely no need for hungarian notation. So maybe that is something to keep in mind - the development environment may contribute to the decision on naming conventions.

Answer 2

You've already received a very good answer, that I completely agree with. However, I want to mention this just in case you really want to stick with the error handling pattern that you're using right now. (I recommend you don't by the way.)

There's a bit of code here that essentially does nothing.

'Attempt to set an object to reference the mymodel object instance
On Error Resume Next
Err.Clear
Set tempModel = MyMODEL
On Error GoTo 0

Any time you call an On Error statement, the global Err object gets reset. There should be no need to Clear it yourself.

Also, what's with all the whitespace here?

'Declare variables
 Dim strURL As String
 Dim blnExists As Boolean

I have to look half way across the screen to see what type these variables are. Close the gap.

'Declare variables
Dim strURL As String
Dim blnExists As Boolean

And ditch the hungarian notation. The variable names more or less tell me what type these are without those prefixes.

'Declare variables
Dim url As String
Dim exists As Boolean

But.... what exists? Let me just look a few lines up... OH! A record exists! Why don't we just say so?

Dim recordExists As Boolean

• CBRF23: I like to have all my variable types in a column, makes it easier for me to quickly scan a list and see what is what. Personal preference I guess.
• RubberDuck: Not really. It can turn the code into a hot mess when copy/pasted if there's a mixture of spaces & tabs.
• Ismael Miguel: @RubberDuck Notepad++ fixes it.