Logical Predicate Class

For a while now I've been playing around with various ways of passing a logical predicate E.G. >= someValue as an argument to a function. (see: If ComparisonIsTrue(thisPost, ComparisonOperator.NotEqualTo, goodCode) Then Me.Answer and a global enum for comparison operators)

In the end, I've decided to build a CLS_Comparison_Predicate class with Operator and RightValue properties, with a Compare() function that takes a LeftValue as an argument.

Thoughts?

Example Usage:

Dim predicate As CLS_Comparison_Predicate
Set predicate = New CLS_Comparison_Predicate

predicate.Operator = GreaterThan
predicate.RightValue = 9000

If predicate.Compare(inputValue) Then
    MsgBox "It's over 9,000!"
End If

Enum:

Public Enum ComparisonOperator
    NotEqualTo = 0
    LessThan = 1
    LessThanOrEqualTo = 2
    EqualTo = 3
    GreaterThanOrEqualTo = 4
    GreaterThan = 5
End Enum

Code:

```
Option Explicit

Private pOperator As ComparisonOperator
Private pRightValue As Variant

Private Const NULL_ERROR_TEXT As String = "Invalid Compare input. Cannot compare against Null"
Private Const OBJECT_ERROR_TEXT As String = "Invalid compare input. Input must be a value, not an object"

Public Property Let Operator(ByVal inputOperator As ComparisonOperator)
pOperator = inputOperator
End Property

Public Property Let RightValue(ByVal inputValue As Variant)
If IsNull(inputValue) Then
PrintErrorMessage NULL_ERROR_TEXT
Stop
ElseIf IsObject(inputValue) Then
PrintErrorMessage OBJECT_ERROR_TEXT
Stop
End If

pRightValue = inputValue
End Property

Public Function Compare(ByVal leftValue As Variant) As Boolean

If IsNull(leftValue) Then
PrintErrorMessage NULL_ERROR_TEXT
Stop
ElseIf IsObject(leftValue) Then
PrintErrorMessage OBJECT_ERROR_TEXT
Stop
End If

Dim isTrue As Boolean
Select Case pOperator
Case NotEqualTo
isTrue = (leftValue <> pRightValue)

Stop

This instruction is outright dangerous in any production code (assuming that code could be used in anything that ends up in the hands of users that don't understand what's happening when the VBE pops up). And because it's easy to forget about it and remove every instance of it everywhere, I usually reserve it for debugging only - for example:

CleanFail:
    Stop
    Resume CleanExit
    Resume
End Sub

When debugging, execution breaks at the Stop keyword, and then I can drag the yellow "next instruction" arrow to Resume and jump back at the error-causing line.

If you're going to be outputting an error message and stop execution, you might as well raise an error to do so. Runtime error 5 "Invalid procedure call or argument" is more or less analoguous to .NET's ArgumentException, which seems to be the most accurate existing error to throw/raise here - I'd do it like this:

Err.Raise 5, TypeName(Me), NULL_ERROR_TEXT

That will only PrintErrorMessage if the caller handles the runtime error though, which changes the way your code works. But it's more semantically correct I find.

Same here:

Case Else
        '/ TODO: Error Handling
        Stop

The function received an invalid argument; there's an existing runtime error specifically for this: just use it!

Public Enum ComparisonOperator
    NotEqualTo = 0
    LessThan = 1
    LessThanOrEqualTo = 2
    EqualTo = 3
    GreaterThanOrEqualTo = 4
    GreaterThan = 5
End Enum

Every single enum value here has the same value is would have if an explicit value wasn't specified, and the values themselves are meaningless. Better leave 'em out and only specify explicit values when these values mean something or can be combined (and then they would be valued with powers of 2).

Objects are indeed not comparable. But not because they're objects - because in and by themselves, they don't specify how they should be compared. The .NET framework solved this problem by introducing a fairly simple interface, IComparable:

Option Explicit

Public Function CompareTo(ByVal other As Variant) As Integer
End Function

Given an object that implements this interface, your code would be able to evaluate all operators, assuming both operands are of the same type; so instead of systematically refusing to compare objects, you could instead raise an error that says something like "Both objects must be the same type and implement IComparable".

Private pOperator As ComparisonOperator
Private pRightValue As Variant

I've seen p used to mean "parameter". Not sure what it stands for here, as typically the Hungarian prefix would be m_ for module-scope variables.

I like to solve this problem like this:

Private Type TComparer
    Operator As ComparisonOperator
    RightValue As Variant
End Type
Private this As TComparer

And then you don't need prefixes anymore:

Public Property Let Operator(ByVal value As ComparisonOperator)
    this.Operator = value
End Property

Notice I'm using a general-purpose value parameter in property setters (let/set); this makes properties read consistently, and pretty much identical to modern VB.

Public Property Let RightValue(ByVal value As Variant)
    If IsNull(value) Then
        PrintErrorMessage NULL_ERROR_TEXT
        Stop
    ElseIf IsObject(value) Then
        PrintErrorMessage OBJECT_ERROR_TEXT
        Stop
    End If

    this.RightValue = value
End Property

Are you sure IsNull is doing what you're using it for here?


The Null value indicates that the Variant contains no valid data. Null is not the same as Empty, which indicates that a variable has not yet been initialized. It is also not the same as a zero-length string (""), which is sometimes referred to as a null string.


https://msdn.microsoft.com/en-us/library/office/gg278616.aspx

It's actually pretty hard to supply a Null value by accident to a required Variant parameter. I'm not saying the check isn't needed, just that it's rather uncommon in VBA code. I'm not even sure how one would manage to actually pass a null in there without explicitly specifying Null.

It's good that you're checking for Null. But then, why stop there? Why not check for vbEmpty? If one can pass Null, they could also pass vbEmpty... and how would you compare against vbEmpty?

StackExchange Code Review Q#128578, answer score: 6