views:

742

answers:

4

Say I have a Point2 class, and I want to implement the following Equals:

public override bool Equals ( object obj )

public bool Equals ( Point2 obj )

This is from the Effective C# 3 book:

public override bool Equals ( object obj )
{
    // STEP 1: Check for null
    if ( obj == null )
    {
     return false;
    }

    // STEP 3: equivalent data types
    if ( this.GetType ( ) != obj.GetType ( ) )
    {
     return false;
    }
    return Equals ( ( Point2 ) obj );
}

public bool Equals ( Point2 obj )
{
    // STEP 1: Check for null if nullable (e.g., a reference type)
    if ( obj == null )
    {
     return false;
    }
    // STEP 2: Check for ReferenceEquals if this is a reference type
    if ( ReferenceEquals ( this, obj ) )
    {
     return true;
    }
    // STEP 4: Possibly check for equivalent hash codes
    if ( this.GetHashCode ( ) != obj.GetHashCode ( ) )
    {
     return false;
    }
    // STEP 5: Check base.Equals if base overrides Equals()
    System.Diagnostics.Debug.Assert (
     base.GetType ( ) != typeof ( object ) );

    if ( !base.Equals ( obj ) )
    {
     return false;
    }

    // STEP 6: Compare identifying fields for equality.
    return ( ( this.X.Equals ( obj.X ) ) && ( this.Y.Equals ( obj.Y ) ) );
}

Is that an overkill?

+1  A: 

Looks to me like exactly what you would want. That whole block boils down to:

"if it's the exact same instance, return true. If they're separate instances with the same X and Y values, return true. All other cases (null, different type, different x/y values) return false."

Dave Swersky
+2  A: 

Not really - you are accounting for pretty much every possibility. If this code is for anything other than a scratch application you should consider the benefits of this approach because logical errors due to weird object equality behavior are painful to debug.

Andrew Hare
+1  A: 

Its certainly more code than I'd want to write for an equals method. There are a lot of redundant checks such as checking for ReferenceEquals and HashCodes (I know these checks are redundant because the last lines in your function check for structural equality). Focus on code which is simple and readable.

public bool Equals(object o)
{
    Point2 p = o as Point2;
    if (o != null)
        return this.X == o.X && this.Y == o.Y;
    else
        return false;
}

Since your equals method uses structural equality, make sure you override GetHashCode with an implementation based on your fields as well.

Juliet
+2  A: 

Supporting equality with an inheritance hierarchy is tricky. You need to work out exactly what you mean. Do you really need inheritance here? If not - if Point2 derives directly from System.Object, and you can make it sealed, life becomes a bit easier. In that case I would use:

public override bool Equals (object obj)
{
    return Equals(obj as Point2);
}

public bool Equals (Point2 obj)
{
    // STEP 1: Check for null if nullable (e.g., a reference type)
    // Note use of ReferenceEquals in case you overload ==.
    if (object.ReferenceEquals(obj, null))
    {
        return false;
    }

    // STEP 2: Check for ReferenceEquals if this is a reference type
    // Skip this or not? With only two fields to check, it's probably
    // not worth it. If the later checks are costly, it could be.
    if (object.ReferenceEquals( this, obj))
    {
        return true;
    }

    // STEP 4: Possibly check for equivalent hash codes
    // Skipped in this case: would be *less* efficient

    // STEP 5: Check base.Equals if base overrides Equals()
    // Skipped in this case

    // STEP 6: Compare identifying fields for equality.
    // In this case I'm using == instead of Equals for brevity
    // - assuming X and Y are of a type which overloads ==.
    return this.X == obj.X && this.Y == obj.Y;
}
Jon Skeet
Thanks Jon. If Point2 inherits from another class, would you use the code I posted fully?
Joan Venge
@Jon - why the check for reference equality (step 2)? Wouldn't that be covered by the actual equality logic of the type (like step 6. above)? Is it to provide a shortcut for improved perf?
Mohit Chakraborty
@Mohit: Yes, it's a shortcut - there's no point in checking individual fields if the references are the same. Whether it's useful or not depends on the checks involved.
Jon Skeet
As per the Liskov substitution principle (http://en.wikipedia.org/wiki/Liskov_substitution_principle), I'd say Point2 classes can be equal to descendent classes.
Juliet
@Joan: If Point2 inherits from something else, I'd consider *very* carefully what I'd want to happen in the case of point1.Equals(point2) and point2.Equals(point1) bearing in mind that object.Equals is meant to be reflexive. Read Effective Java (2nd edition) for more on this.
Jon Skeet