views:

162

answers:

3

Background
Let's say I have the following class:

public class MyValue<T>
{
    public T Value { get; set; }    
    public static bool operator ==(MyValue<T> first, MyValue<T> second)
    {
        // if first and second are the same instance, they are equal
        if (object.Equals(first, second))
        {
            return true;
        }

        // for each of the objects, get a value indicating whether either
        // the object or its Value property is null
        bool firstIsNull = object.Equals(first, null) ? true : first.Value == null;
        bool secondIsNull = object.Equals(second, null) ? true : second.Value == null;

        // if both are null, they are equal
        if (firstIsNull && secondIsNull)
        {
            return true;
        }

        // Not both are null. If one is, they are not equal
        if (firstIsNull || secondIsNull)
        {
            return false;
        }

        // Neither first nor second is null; compare their values
        return first.Value.Equals(second.Value);
    }    
    // more code implementing !=, Equals, GetHashCode, implicit conversions and so on
}

This class will be used as the type on properties in an object model. An object could look like so:

public class SomeClass
{

    public SomeClass()
    {
        SomeValue = new MyValue<string>();
    }
    public MyValue<string> SomeValue { get; set; }
}

The idea with this is that the class simply represents the value of its Value property. It has some more functionality that is stripped away since it's not relevant for the question (some validation and stuff). Since the class has no value in itself, we want it to be as little intrusive as possible when used. Implicit conversions allows for code like this:

SomeClass instance = new SomeClass();
instance.SomeValue = "my string";

...and the question:
The idea with overloading the == (and !=) operator is to have an instance of the object where Value is null to compare equal to null (partly because we feel it makes sense, partly because of backwards compatibility issues in the object model):

MyValue<string> a = new MyValue<string>();
MyValue<string> b = null;
bool areTheyEqual = (a == b); // this will be true

This may of course seem a bit confusing, but given that MyClass is simply a wrapper, that in most of the code will be rather invisible, does it make sense to you too, or is it something really bad that we will regret for reasons that we overlooked?

A: 

If you override ==, then normally it is best to override .Equals to use the same logic.

ck
@ck: that is already done; I stripped it away to reduce the amount of code (as indicated by the last code comment in the sample). Good point though.
Fredrik Mörk
+9  A: 

In my opinion, == operator in C# is already confusing enough. If you overload it like that, you'll be increasing the possibility of confusion.

Class c = somethingEqualsToNullButNotNull;
if (c == null) { // true
}

object co = c;
if (co == null) { // false. WTF!?
}
Mehrdad Afshari
Yes, that is more than a little confusing. Very strong argument against the approach.
Fredrik Mörk
+1. Despite the name, this looks like a mutable reference type. Microsoft recommends avoiding overloading equality operators on those.
TrueWill
+6  A: 

You say you've already overridden Equals to provide the same behaviour. That means you've already broken the contract for Equals, which explicitly states:

- x.Equals(a null reference (Nothing in Visual Basic)) returns false.

Basically: no, don't do this. It will be confusing to users of the class.

Now Nullable<T> appears to break this rule:

int? x = new int?(); // Or int? x = null;
object o = null;
Console.WriteLine(x.Equals(o)); // True

... but in this case x isn't a reference, which is part of the contract description. The only way of making x a reference is to box the value, at which point x is null anyway. In your case, MyValue is a class so you can have a non-null reference which violates the contract.

Jon Skeet
This is also a very strong argument against the solution. I guess we just need to do some refactoring of our old code to have it play nicely with the wrapper.
Fredrik Mörk
I happened to stumble in here again, and I just thought of something. While I wholeheartedly agree with your answer, I also realized that `Nullable<T>` actually works only because it does break the `Equals` contract: `int? n = null; Console.WriteLine(n.Equals(null));` (prints `true`) (and obviously `n` is not a null reference, since `Equals` is successfully invoked). Or am I missing something fundamental?
Fredrik Mörk