Skip to main content
Commonmark migration
Source Link

###Nitpicks

Nitpicks

###Reference Check

Reference Check

###Error Handling

Error Handling

###Nitpicks

###Reference Check

###Error Handling

Nitpicks

Reference Check

Error Handling

Source Link
Mathieu Guindon
  • 75.1k
  • 18
  • 191
  • 463

###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.