Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

This is a follow-on to a global enum for comparison operators. I wanted to pass a logical expression E.G. >= someValue as a parameter to a function, so I made an Enum and an accompanying function to analyse logical expressions.


Is my approach a good way to implement this functionality in VBA?


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

Public Function ComparisonIsTrue(ByVal sourceValue As Variant, ByVal operator As ComparisonOperator, ByVal comparisonValue As Variant) As Boolean

    If IsNull(sourceValue) Or IsNull(comparisonValue) Then GoTo CleanFail
    If IsObject(sourceValue) Or IsObject(comparisonValue) Then GoTo CleanFail

    Dim isTrue As Boolean

    Select Case operator

        Case NotEqualTo
        isTrue = (sourceValue <> comparisonValue)

        Case LessThan
        isTrue = (sourceValue < comparisonValue)

        Case LessThanOrEqualTo
        isTrue = (sourceValue <= comparisonValue)

        Case EqualTo
        isTrue = (sourceValue = comparisonValue)

        Case GreaterThanOrEqualTo
        isTrue = (sourceValue >= comparisonValue)

        Case GreaterThan
        isTrue = (sourceValue > comparisonValue)

        Case Else
        '/ Error Handling
        Stop

    End Select

CleanExit:
    ComparisonIsTrue = isTrue
    Exit Function

CleanFail:
    isTrue = False
    GoTo CleanExit

End Function

share|improve this question
up vote 5 down vote accepted

Invalid input

Consider the following example (Example 1): You have a cell A1 containing the value 3. Now assume the following call:

result = ComparisonIsTrue(ActiveWorkbook.ActiveSheet.Range("A1"), EqualTo, 3)   

' result is now False

I made an error and accidentally passed a Range object instead of the value. Now, your method hides that error and just returns the wrong result. Instead, I would suggest that you throw an error with a helpful error message if an invalid value is passed. That way, you help the developer find his error instead of hindering him by returning an arbitrary result:

If IsObject(sourceValue) Or IsObject(comparisonValue) Then 
    Err.Raise vbObjectError, , "sourceValue and comparisonValue must be value types."
End If

As an alternative, there is a "trick" you can use for cases like the one above: VBA has a feature called Let coersion. Let's consider a modified version of the example again (Example 2):

Dim x As Variant
x = ActiveWorkbook.ActiveSheet.Range("A1")

result = ComparisonIsTrue(x, EqualTo, 3)   

' result is now True

Since x is assigned without the Set keyword, the Range object is converted to its "simple data value". You can use the same trick inside your method to implicitly convert objects to their data representation:

Public Function ComparisonIsTrue(ByVal sourceValue As Variant, ByVal operator As ComparisonOperator, ByVal comparisonValue As Variant) As Boolean

    ' Convert objects to their "simple data value", if necessary.
    sourceValue = sourceValue
    comparisonValue = comparisonValue

That way, even Example 1 would return the "expected" result True.

Indentation

Select-Case statements should be indented as follows:

Select Case expression
    Case case1
        Do something
    Case case2
        Do something else
...
share|improve this answer

I really don't like your GoTos...

If IsNull(sourceValue) Or IsNull(comparisonValue)
Or IsObject(sourceValue) Or IsObject(comparisonValue) Then
    ComparisonIsTrue = False
    Exit Function
End If

And gone are your gotos.

share|improve this answer
1  
You are right hat, In general, GoTo should be avoided. However, in languages like VBA which don't have finally blocks, GoTo is a legitimate (and, in fact, the only DRY) way to jump to the "clean up and exit" block. – Heinzi Mar 11 at 19:56

A more "standard" approach would be a Predicate. Since my VBA is gruesome here's a C-Style implementation

class Predicate<T> {
    ComparisonOperator operator;
    T checkValue;

    boolean test(T value) {
       // your switch goes here
    }
}

This allows you to encapsulate and pass a single object instead of two for testing.

Your title would look something like this in the end:

Dim codeReviewCheck As Predicate
codeReviewCheck.Operator = ComparisonOperator.NotEqualTo
codeReviewCheck.TestValue = GoodCode

If codeReviewCheck.Test(thisPost) Then
    Me.Answer
End If

This is also closer to your original intent:

I wanted to pass a logical expression E.G. >= someValue as a parameter to a function, [..]

Currently you have to pass two parameters. That's not simplistic and leads to parameter bloat on your methods. Encapsulate things that belong together into one single class

share|improve this answer
3  
Great idea, but you need function pointers & generics to make that work properly. VBA has neither. – RubberDuck Mar 11 at 17:13
1  
since the original is working on a Variant anyways, the Generics are actually optional. It's just the C-Style implementation that benefits from these – Vogel612 Mar 11 at 17:17

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.