Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I've implemented the Equals() support for my class as follows:

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

public bool Equals(TwoDPoint p)
{
  return ((object)p != null) && (x == p.x) && (y == p.y);
}

This obviously doesn't match the reference implementation on MSDN, here, as they add explicit checking for null before and after converting to a TwoDPoint, and they implement the actual equality test in each method (i.e. no call from one to the other).

I like my implementation as it's much more succinct. But I'm wondering - what have I missed? Am I losing (significant) performance with my version? Is there actually a bug in this approach?

share|improve this question
2  
Breaking up relatively complex code in multiple statements on multiple lines, make it easier to step through the code in a debugger. And set breakpoints. That alone may warrant a more verbose coding style. – Thorbjørn Ravn Andersen Mar 10 at 19:35
1  
Not sure you'd ever need to set individual breakpoints for a simple three value-test conditional (but would tend to agree with you in the general case). – Rob Gilliam Mar 10 at 20:13
    
There might be a coding style guide which has to be followed to make code more similar regardless of who wrote it. Stranger things has happened. – Thorbjørn Ravn Andersen Mar 10 at 20:23
    
For performance you want a struct and not a class. – Johan Larsson Mar 10 at 20:28
4  
You'll know if the performance difference is significant after you do two things: (1) carefully define significance, and (2) carefully measure. Anything else is guessing. – Eric Lippert Mar 10 at 21:57
up vote 13 down vote accepted

I like my implementation as it's much more succinct

Which isn't necessarily a good thing: we care about readable code, not about oneliners. It means we don't use 5 lines to write what can be written in 1 line but it also means we don't cram 5 lines in 1 line just for the sake of it.

That being said: your implementation does the same as the example. I would also prefer your example since it keeps the equality logic in one method Equals(T) rather than duplicating it.

Some comments however:

  • There's no point in doing this cast: (object)p != null
  • You might as well implement IEquatable<T> MSDN
  • There should be no performance difference unless you have an unusual amount of null arguments to Equals(object) that will now go through the as statement before being caught by the null check
  • Equals(T) will probably be inlined anyway
share|improve this answer
4  
"There's no point in doing this cast: (object)p != null" but I just read in the documentation where they write about infinite loops due to != or == triggering Equals. – Pimgd Mar 10 at 15:54
3  
@Pimgd, IMO !object.ReferenceEquals(p, null) would be a more self-documenting way of making the same check without risking loops due to operator != implementations. – Peter Taylor Mar 10 at 16:32
1  
The cast of (object)p != null looks unnecessary and even tools like R# will tell you that it is, but infinite loops invariably occur without it. You need to do it every time you provide implementations for class equality – NWard Mar 10 at 17:30
1  
@NWard My understanding was that it'll only lead to infinite loops if you override the == operator. – Ben Aaronson Mar 10 at 17:45
1  
Overriding .Equals() without overriding == sounds like a hazardous game, but I suppose you could have cases where you don't need to. I like it when both my equality operations are consistent. – NWard Mar 10 at 17:50

The benefit of explicit null checking is that if the object is null, you return false without the performance impact of casting the object to your type.

share|improve this answer
    
Good point, although as Jeroen mentions this is only an issue if you expect a lot of nulls. In my actual case I expect none at all (but you gotta implement for it, 'cos if you don't, you know what'll happen!) – Rob Gilliam Mar 10 at 20:14

Warning

void Main()
{
    bool testOutcome;
    var testObject1 = new RecursiveEquals();
    var testObject2 = new RecursiveEquals();

    //// These will work
    testOutcome = (object)testObject1 == testObject2;
    testOutcome = (object)testObject1 != testObject2;

    //// These all crash
    //  testOutcome = testObject1 == testObject2;
    //  testOutcome = testObject1 != testObject2;
    //  testOutcome = testObject1.Equals(testObject2);
    //  testOutcome = ((object)testObject1).Equals(testObject2);
}

class RecursiveEquals
{
    public override bool Equals(object other)
    {
        // this would typically be less obvious
        return other.Equals(this);
    }

    public static bool operator ==(RecursiveEquals left, object right)
    {
        return ((object)left).Equals(right);
    }

    public static bool operator !=(RecursiveEquals left, object right)
    {
        return !((object)left).Equals(right);
    }
}

Casting p as a less derived object will NOT invoke the less derived implementation of the method.

I've tried to make the recursive .Equals call obvious enough to illustrate what is a more subtle common problem when people override it. In this answer, I am focusing on the point that casting the object does not change the method implementation which is used.

Unless casting to an interface type which is implemented explicitly in the Class's hierarchy, this will invoke the most derived definition. To not work this way would violate polymorphism principles.

This will work for operator overloads in .NET as you are doing. Until I started researching this answer, I was not aware that operator overloads do NOT actually use the most derived implementation. I personally find this result surprising.

share|improve this answer
    
You've got a stackoverflow because Equals(o) calls Equals(o). I don't see what the point of this answer is – Jeroen Vannevel Mar 10 at 22:00
    
Yes, presenting a StackOverflow condition on CodeReview is ironic. Updated answer to try to more clearly focus the point. – psaxton Mar 10 at 22:09
    
I'm also (still) not sure what the point of this answer is. (I undertand the point you're making, I'm just not sure why you're making it here!) – Rob Gilliam Mar 11 at 11:12
    
There are a couple comments on the accepted answer questioning the legitimacy of casting to object for the inequality operator. Originally I was going to show that it would use the same implementation, causing the SO condition they were trying to avoid. I found the result surprising enough I wanted to document it here for anyone else. – psaxton Mar 11 at 13:16

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.