Is this a good/efficient idiom for implementing Equals and equality/inequality operators?

Question

I have had a few problems getting this right, so I wanted to ask if anyone has any feedback on whether this is an efficient way to implement the Equals method and equality/inequality operators for a custom immutable class. These operators are called very frequently by my program, so I want to make sure I get them right.

class MyObj
{

    public static bool operator ==(MyObj a, MyObj b)
    {
        if (!object.ReferenceEquals(a, null))
            return a.Equals(b);
        else if (!object.ReferenceEquals(b, null))
            return b.Equals(a);
        else
            // both are null
            return true;
    }

    public static bool operator !=(MyObj a, MyObj b)
    {
        if (!object.ReferenceEquals(a, null))
            return !a.Equals(b);
        else if (!object.ReferenceEquals(b, null))
            return !b.Equals(a);
        else
            // both are null
            return false
    }

    public override bool Equals(object obj)
    {
        return this.Equals(obj as MyObj);
    }

    public bool Equals(MyObj obj)
    {
        if (object.ReferenceEquals(obj, null))
            return false;
        else
            return (obj.FieldOne == this.FieldOne &&
                    obj.FieldTwo == this.FieldTwo && ...);
    }

}

Answer 1

I use the following code snippet for reference types, which has less duplication and feels cleaner, in my opinion. Having a static "Equals" method allows .NET languages without operator overloading to compare your instances without having to test for null before calling the instance method. If you're implementing equality operators, it might also be best to make your class immutable, if you can.

class Foo : IEquatable<Foo>
{
    public override bool Equals(object obj)
    {
        return Equals(obj as Foo);
    }

    public bool Equals(Foo other)
    {
        if (object.ReferenceEquals(other, null)) return false;

        // Optional early out
        if (object.ReferenceEquals(this, other)) return true; 

        // Compare fields here
    }

    public static bool Equals(Foo a, Foo b)
    {
        if (ReferenceEquals(a, null)) return ReferenceEquals(b, null);
        return a.Equals(b);
    }

    public static bool operator ==(Foo a, Foo b)
    {
        return Equals(a, b);
    }

    public static bool operator !=(Foo a, Foo b)
    {
        return !Equals(a, b);
    }
}

Answer 2

Basically yes, but there is an error to correct.

The Equals(object) method calls itself instead of calling the Equals(MyObj) method, causing an eternal loop. It should be:

public override bool Equals(object obj) {
   MyObj other = obj as MyObj;
   return this.Equals(other);
}

or simply:

public override bool Equals(object obj) {
   return this.Equals(obj as MyObj);
}

Also, you can simplify the inequality operator to:

public static bool operator !=(MyObj a, MyObj b) {
   return !(a == b);
}

Answer 3

Some things I'm noticing:

• Because you're overriding Equals, you should also override GetHashCode.
• Since your Equals(MyObj) method is a valid implementation for the entire IEquatable<MyObj> interface, MyObj should indeed implement that interface. This will also allow Dictionary<> and such to directly take advantage of your Equals(MyObj) method, instead of going through Equals(object).

Also I completely agree with Trillian's alternative implementation, except I would've implemented a != b directly as !(a == b) instead of !Equals(a, b). (Trivial difference of course.)

Answer 4

If you're looking for efficiency, I recommend using this instead of object.ReferenceEquals(foo, null):

(object)foo == null

This is effectively equivalent but avoids a function call.

I also like to implement IEquatable<T> in all my types that override Equals. For reference types, I then forward Equals(object) to Equals(Foo).

public override bool Equals(object other){return Equals(other as Foo);}

The operator overloads can be simplified as so:

public static bool operator==(Foo a, Foo b){
    if((object)a == null)
        return (object)b == null;
    return a.Equals(b);
}
public static bool operator!=(Foo a, Foo b){
    return !(a == b);
}

If absolute efficiency is needed, though, it may be worth a little duplication of code in these functions to avoid the extra function calls, but unlike using (object)foo == null instead of object.ReferenceEquals(foo, null), avoiding the function call requires extra code to maintain, so the small gain may not be worth it.

Answer 5

I prefer to leave all the "if this is null then do that else..." logic to the framework:

class MyObj : IEquatable<MyObj> {

  public static bool operator ==( MyObj left, MyObj right ) {
    return EqualityComparer<MyObj>.Default.Equals( left, right );
  }

  public static bool operator !=( MyObj left, MyObj right ) {
    return !EqualityComparer<MyObj>.Default.Equals( left, right );
  }

  public override bool Equals( object obj ) {
    return this.Equals( obj as MyObj );
  }

  public bool Equals( MyObj other ) {
    return !object.ReferenceEquals( other, null )
        && obj.FieldOne == this.FieldOne
        && obj.FieldTwo == this.FieldTwo
        && ...
        ;
  }

  ...

}

See also What is the best algorithm for an overridden GetHashCode? for implementing GetHashCode.