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