IEquatable - Best practice override for .Equals(object obj) in C#

Question

Whenever I write a new class or struct that is likely to hold some data, that may need to be compared, I always implement IEquatable<T> as this provides the class/struct with a strongly typed .Equals(T other) method.

example:

public struct Radius : IEquatable<Radius>
{
    public Int32 TopLeft { get; set; }
    public Int32 TopRight { get; set; }
    public Int32 BottomLeft { get; set; }
    public Int32 BottomRight { get; set; }

    public bool Equals(Radius other)
    {
        return this.TopLeft == other.TopLeft
            && this.TopRight == other.TopRight
            && this.BottomLeft == other.BottomLeft
            && this.BottomRight == other.BottomRight;
    }
}

As well as providing an implementation for .Equals(Radius other), I should really override the default implementation too (.Equals(object obj))

I have two options here, and my question is, which of these implementations is better?

Option 1 is to use casting:

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

Option 2 is to use the "as" keyword:

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

My reason for asking this is, using casting will throw an exception if obj cannot be cast to Radius, whereas as will resolve to null if it cannot be cast, therefore it just checks this against null, without throwing an exception; So is it better to throw an exception, or to just return false?

EDIT: As it has been pointed out by quite a few fellow SO'ers, structs cannot be null, therefore the second option does not apply for a struct. Therefore another question springs to mind: Should the overridden implementation of .Equals(object obj) be identical for structs and classes?

Your `Equals(Radius)` method should return false if `other` is null, otherwise it will throw an exception with null input. — cdhowie, Aug 07 '13 at 15:36
You can't use the `as` keyword as `Radius` isn't a nullable type. You should instead use `is` to return false for other types, and then just cast. — Servy, Aug 07 '13 at 15:38
@Servy I missed that. In that event, it can still be done with one type-check by using an `as`-cast to a nullable type: `var radius = obj as Radius?; return radius == null ? false : Equals(radius.Value);` One should always prefer `as` over `is`-followed-by-parenthetical-cast as it only performs one type check. — cdhowie, Aug 07 '13 at 15:39
@Servy, I haven't tested this, but then why not " obj as Radius? "? — Matthew Layton, Aug 07 '13 at 15:42
@cdhowie Yes, it can, but there's no real advantage (or disadvantage) to doing so. An additional type check is just as expensive as an additional null check. — Servy, Aug 07 '13 at 15:42
@series0ne Because `Equals` doesn't take a `Radius?`, it takes a `Radius`. — Servy, Aug 07 '13 at 15:42
@Servy No, a null check is an equality test of a pointer-sized value, which can be done in one x86 instruction. `is` is demonstrably slower. http://stackoverflow.com/questions/686412/c-sharp-is-operator-performance (This could be considered a micro-optimization, but personally I find `as` to be more readable in this case. `is` should only be used if you want to know if an object is of a type but don't plan to cast it to that type immediately.) — cdhowie, Aug 07 '13 at 15:45
@cdhowie A 30% increase in speed of one of the fastest possible operations that can be performed. That's an entirely negligible difference in performance, to the point that even mentioning it isn't worthwhile in virtually any application, ever. Using `as` would make sense if the type itself were nullable, and you wanted the nullable result, but sense it's not logically nullable the work you're doing to translate it back effectively removes the advantage gained. The two solutions are quite equivalent, the differences are almost entirely subjective preference, no more. — Servy, Aug 07 '13 at 15:49
How about `var otherRad = other as IEquatable; if (otherRad != null) return otherRad.Equals(this); else return false;`? — supercat, Aug 08 '13 at 23:07

SLaks · Accepted Answer · 2013-08-07T15:39:27.420

7

The Equals() method must never throw an exception.

An object of a different type is merely unequal.

Quoting the documentation:

Implementations of Equals must not throw exceptions; they should always return a value. For example, if obj is null, the Equals method should return false instead of throwing an ArgumentNullException.

edited Aug 07 '13 at 15:39

answered Aug 07 '13 at 15:34

SLaks

868,454
176
1,908
1,964

"An object of a different type" -- or no object at all (`null`). – cdhowie Aug 07 '13 at 15:36
Unless the object you are calling Equals on is null – chris warner Aug 07 '13 at 15:37
1

@chriswarner: In that case, the exception is thrown before entering `Equals()`. – SLaks Aug 07 '13 at 15:38
@SLaks, Of course :s I was looking at the decompiled source for String.Equals, Do you know why that does a null check? is it just a "belt and braces" approach? – chris warner Aug 07 '13 at 15:43
1

@chriswarner: The source says `if (this == null) //this is necessary to guard against reverse-pinvokes and throw new NullReferenceException(); //other callers who do not use the callvirt instruction` – SLaks Aug 07 '13 at 15:44
@SLaks: Many thanks, I didnt get the comments through resharper but that makes sense – chris warner Aug 07 '13 at 15:48
@chriswarner Note that this test is required for String due to the way strings are actually stored -- the character data is allocated as part of the String object itself, not as an array. This optimization allows the runtime to make one allocation for String objects instead of two (String and character array). Because of this, the String class uses unsafe character pointers to access the character data. There is no reason you would have to do a `this == null` test in your own Equals() methods, as you would automatically throw an NRE when you tried to use `this`. – cdhowie Aug 07 '13 at 15:56
@cdhowie: AFAIK, that has nothing to do with it. The CLR team wanted to be more resilient against non-virtual method calls that don't check `this`. – SLaks Aug 07 '13 at 15:58
@cdhowie: `var d = (Func – SLaks Aug 07 '13 at 16:00
@SLaks Unless this changed recently then it is true. Strings have a private `m_firstChar` field, which is the *beginning* of the character data. To read beyond the first character, a `char*` is taken to this field and then incremented. This is the reason for the null test -- if one doesn't test if `this == null` then `char* c = this.m_firstChar` will succeed and return a non-null pointer. Dereferencing it will then likely cause an access violation. – cdhowie Aug 07 '13 at 16:02
@cdhowie: I clarified my comment. That is true, but it has nothing to do with `this` being `null`. – SLaks Aug 07 '13 at 16:02
1

@SLaks It has a great deal to do with it when one is using pointers. In safe code, a null test is not necessary as using a member of a null object will raise a NullReferenceException, which is perfectly acceptable in the face of broken code. An access violation caused by pointer arithmetic on a null object is not acceptable. – cdhowie Aug 07 '13 at 16:04
@cdhowie: I see; you're right. (actually, the first thing `Equals()` does is check `this.Length`, which, comments say, is inlined to a single instruction. (so it would presumably die hard without the null check) – SLaks Aug 07 '13 at 16:10
let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/35013/discussion-between-cdhowie-and-slaks) – cdhowie Aug 07 '13 at 16:11
It's best practice to also override `GetHashCode` if you override `Equals`, isn't it? – Tim Schmelter Jul 21 '14 at 12:13
@TimSchmelter: Yes; it's required. – SLaks Jul 21 '14 at 20:36
I stand to not throwing exceptions here. Remember you do not always consciously know when this method will be called and will definitely not be expecting an exception in many of the scenarios it can be called in. – Thulani Chivandikwa Jul 31 '15 at 09:29

score 5 · Answer 2 · answered Aug 08 '13 at 09:12

As @SLaks already mentioned Equals() should never throw.

In this special case i think a usage of the is operator combined with a cast should help you out:

public override bool Equals(object obj)
{
     if(obj is Radius)
         return Equals((Radius)obj);

     return false;
}

In cases where you have a class you should simply use the as operator:

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

public bool Equals(MyObj obj)
{
     if(ReferenceEquals(obj, null))
         return false;

     // ToDo: further checks for equality.
}

score -1 · Answer 3 · answered Aug 07 '13 at 15:35

-1

My personal opinion would be to use the second option, or even check before hand if the object is a "Radius" and then return false so that the intent is more clear

answered Aug 07 '13 at 15:35

chris warner

178
6

IEquatable - Best practice override for .Equals(object obj) in C#

3 Answers3