5
\$\begingroup\$

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?

\$\endgroup\$
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – Thorbjørn Ravn Andersen Mar 10 '16 at 19:35
  • 1
    \$\begingroup\$ 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). \$\endgroup\$ – Rob Gilliam Mar 10 '16 at 20:13
  • \$\begingroup\$ 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. \$\endgroup\$ – Thorbjørn Ravn Andersen Mar 10 '16 at 20:23
  • \$\begingroup\$ For performance you want a struct and not a class. \$\endgroup\$ – Johan Larsson Mar 10 '16 at 20:28
  • 4
    \$\begingroup\$ 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. \$\endgroup\$ – Eric Lippert Mar 10 '16 at 21:57
13
\$\begingroup\$

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
\$\endgroup\$
  • 4
    \$\begingroup\$ "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. \$\endgroup\$ – Pimgd Mar 10 '16 at 15:54
  • 3
    \$\begingroup\$ @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. \$\endgroup\$ – Peter Taylor Mar 10 '16 at 16:32
  • 1
    \$\begingroup\$ 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 \$\endgroup\$ – NWard Mar 10 '16 at 17:30
  • 1
    \$\begingroup\$ @NWard My understanding was that it'll only lead to infinite loops if you override the == operator. \$\endgroup\$ – Ben Aaronson Mar 10 '16 at 17:45
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – NWard Mar 10 '16 at 17:50
7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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!) \$\endgroup\$ – Rob Gilliam Mar 10 '16 at 20:14
-2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ You've got a stackoverflow because Equals(o) calls Equals(o). I don't see what the point of this answer is \$\endgroup\$ – Jeroen Vannevel Mar 10 '16 at 22:00
  • \$\begingroup\$ Yes, presenting a StackOverflow condition on CodeReview is ironic. Updated answer to try to more clearly focus the point. \$\endgroup\$ – psaxton Mar 10 '16 at 22:09
  • \$\begingroup\$ 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!) \$\endgroup\$ – Rob Gilliam Mar 11 '16 at 11:12
  • \$\begingroup\$ 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. \$\endgroup\$ – psaxton Mar 11 '16 at 13:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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