Skip to main content
Code Review

Return to Question

Include missing tests for empty strings
Source Link
this
this
  • 2k
  • 13
  • 24
LeftValue RightValue Result
 1         1          False
 1         0          True
 Null      1          True
 0         Null       True
 Null      Null       False
 ""        ""         False
 ""        Null       True

The special case of an empty string and a Null is arguably problematic. I've swung both ways; sometimes I want empty string to be considered "equal" to a Null, sometimes I don't. In the IsDistinct as defined, they are not considered equal.3

  1. Thanks to @Ryan Wildry for pointing this blind spot out!
LeftValue RightValue Result
 1         1          False
 1         0          True
 Null      1          True
 0         Null       True
 Null      Null       False

LeftValue RightValue Result
 1         1          False
 1         0          True
 Null      1          True
 0         Null       True
 Null      Null       False
 ""        ""         False
 ""        Null       True

The special case of an empty string and a Null is arguably problematic. I've swung both ways; sometimes I want empty string to be considered "equal" to a Null, sometimes I don't. In the IsDistinct as defined, they are not considered equal.3

  1. Thanks to @Ryan Wildry for pointing this blind spot out!
Became Hot Network Question
Tweeted twitter.com/StackCodeReview/status/1202512940592771072
Source Link
this
this
  • 2k
  • 13
  • 24

Simplify & Reduce steps in the IsDistinct function

Because I often have to deal with two Variants that may or may not be Null, we need a null-safe equality test, so I came up with IsDistinct which works but I have some issues with the code:

  1. When I tried to keep it terse, the readability was harmed.
  2. When I tried to expand the logic for readability, it still makes for some thinking.
  3. I looked for potentials to short-circuit or otherwise result the number of steps to arrive at a result. In this case, it all takes 2 evaluations, unless both sides are non-null, in which case we have 3 evaluations.1

Can we do better?

Public Function IsDistinct(LeftValue As Variant, RightValue As Variant) As Boolean
    If IsNull(LeftValue) Then
        If IsNull(RightValue) Then
            IsDistinct = False
        Else
            IsDistinct = True
        End If
    Else
        If IsNull(RightValue) Then
            IsDistinct = True
        Else
            IsDistinct = Not (LeftValue = RightValue)
        End If
    End If
End Function

Inputs & Expected Outputs

LeftValue RightValue Result
 1         1          False
 1         0          True
 Null      1          True
 0         Null       True
 Null      Null       False

Note that it doesn't have to be just 0 and 1; it could be text, dates, or numbers. It's more important that when either inputs are Null, it should automatically be True since Null will always be "distinct" from any non-Null values. However, when both inputs are Null, then it's always False because we are considering them "equal" in this situation.2

  1. In one of iterations, I considered starting with Result = (LeftValue = RightValue) and doing additional evaluation if the Result was Null, signaling that either or both inputs were Null. But IIRC, I found that it made for more steps since I had to evaluate each inputs with IsNull to determine whether both were Null and thus not distinct.

  2. As a matter of fact, when I look at the first 3 lines:

If IsNull(LeftValue) Then
    If IsNull(RightValue) Then
        IsDistinct = False

My instinct is to go "wait, that's not right.", thinking about it and realize it IS correct. The fact that I stumble on that even more than once tells me that it's quite hard to read. Boo.