Inline error checking in VBA

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

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

StackExchange Code Review Q#90738, answer score: 10