in IEquatable implementation is reference check necessary

Question

I have a class that imlements IEquatable<T>. Is it necessary to do a refrence check in Equals() or is it taken care of in the framework?

class Foo : IEquatable<Foo>
{
    int value;
    Bar system;

    bool Equals(Foo other)
    {
        return this == other || 
        ( value == other.value && system.Equals(other.system) );
    }
}

In the above example is the this==other statement superfluous or necessary?

Update 1

I understand I need to correct the code as follows:

    bool Equals(Foo other)
    {
        if( other==null ) { return false; }
        if( object.ReferenceEquals(this, other) ) { return true; } //avoid recursion
        return value == other.value && system.Equals(other.system);
    }

Thanks for the replies.

Answer 1

It's generally an optimization - it would be a strange Equals implementation which would fail without it. I would therefore not regard it as necessary - but nor does it it "taken care of in the framework". I's a cheap optimization to achieve, so it's usually worth including.

Note that if you're also overloading ==, then you should probably use object.ReferenceEquals to perform these comparisons.

Answer 2

Be careful. I would actually strongly discourage this since if you ever want to overload the == operator for your Foo type in terms of Equals (as is usually done in my experience) you'll find yourself with an infinite recursion.

To illustrate what I mean, here is a common implementation of == in terms of Equals:

public static bool operator ==(Foo x, Foo y)
{
    // Will overflow the stack if Equals uses ==:
    return !ReferenceEquals(x, null) && x.Equals(y);
}

That said, I can wholeheartedly agree with Jon's point that it may be appropriate to use ReferenceEquals instead.

Answer 3

It's not necessary in the sense that it may not be required for correctness, but the framework certainly does not "take care of it", so it may be useful to put in, typically for performance reasons.

One point: if the implementation is wrapped by EqualityComparer<T>.Default, it doesn't enter the user code if one or both of the arguments are null, so in that case it does perform some measure of reference checking (if not a full ReferenceEquals(x, y)).

public override bool Equals(T x, T y)
{
    if (x != null)
    {
        return ((y != null) && x.Equals(y));
    }
    if (y != null)
    {
        return false;
    }

    return true;
}

Off-topic, there are several null-dereferencing issues in your sample method (other might be null, this.system might be null).

I would write your method as something like:

public bool Equals(Foo other)
{
    if(other == null)
         return false;

    if(other == this)
         return true;

    return value == other.value
            && EqualityComparer<Bar>.Default.Equals(bar, other.bar)
            // You don't *have to* go down the EqualityComparer route
            // but you do need to make sure you don't dereference null.

}

Also remember to override GetHashCode whenever you write your own equality-comparisons.

Answer 4

I think it is necessary to check this == other, because you define your own equals. If you don't want it to be pointer-checked, you write your own IEquatable.

Answer 5

IEquatable<T> is an interface; the implementation is up to the implementer.

Since you are implementing the interface you are responsible for all expected behavior as defined by the interface.

Answer 6

I believe that it's necessary. The reference check is the first, quick step you can perform when comparing objects.

In the example you have given, be sure to check that other is not null before accessing its values.

bool Equals(Foo other)
{
    if(other == null) return false;

    if(this == other) return true;

    // Custom comparison logic here.
}