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

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.

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