dotnet / dotnet/dotnet-api-docs
Improvements to IEquatable<T>.Equals example
- Dominant language
- C#
- Stars
- 949
- Forks
- 1.7k
- Avg merge
- 3d 27m
- Merged PRs (30d)
- 49
Description
I was studying the [C# sample code](https://github.com/dotnet/samples/blob/master/snippets/csharp/VS_Snippets_CLR_System/system.GenericIEquatable.Equals/cs/EqualsEx2.cs) in the documentation for the `IEquatable.Equals(T)` method and I noticed several issues that does not affect the correctness of the code but at least in my mind makes the code harder to understand and maintain. I want to bring these up here because I believe a sample in the official documentation probably will be used as a template for building a lot of code and so the quality has to be top notch. Now, you may not agree with my observations and I while I would not mind editing the example I think my observations are "subjective" of nature so I would like to get feedback before eventually making changes.
Here are my observations:
# IEquatable.Equals and inheritance
Implementing `IEquatable.Equals` when inheritance is involved is not easy to get right. C# Language Team developer Jared Parsons has [an answer about the problem on Stack Overflow](https://stackoverflow.com/a/1868485/98607). I suggest that the starting point for a class implementing `IEquatable.Equals` is that it should be `sealed`.
# Comparing to null
The methods `public bool Equals(Person other)` and `public override bool Equals(Object obj)` checks to see if a `Person` reference is `null`. This is done using the `==` operator:
if (other == null)
return false;
However, the `Person` class implements the `==` operator so this code will call this operator and some of the code paths for this operator will call `IEqutable.Equals`. To some this may come as a surprise and this can lead to unexpected recursion along some code paths. This does not happen in the sample because the `==` operator correctly checks for `null` but I believe that the chance of introducing bugs increases when the provided `==` operator is used in any of the equality methods.
My suggestion is to always use `object.ReferenceEquals` when checking if a `Person` reference is `null`:
if (ReferenceEquals(other, null))
return false;
Note, that using `==` to compare fields of `Person` is not a problem. It is only when comparing a `Person` reference in a method in the `Person` class.
# Using pattern matching
At least with C# 7.0 you can use pattern matching to make the `object.Equals` method much simpler and hopefully easier to understand and verify for correctness:
public override bool Equals(object obj) => obj is Person person && Equals(person);
# Simplified null checking
In the `==` operator there are checks to ensure that if either of the `Person` references are null then `IEqutable.Equals` is not be called. This avoids the possibility for recursion described above. The code uses casts to `object` to avoid recursively calling itself:
if (((object)person1) == null || ((object)person2) == null)
return Object.Equals(person1, person2);
Again, you have to be mindful of using the correct `==` operator. This can be avoided by using `ReferenceEquals`:
if (ReferenceEquals(person1, null) || ReferenceEquals(person2, null))
return Object.Equals(person1, person2);
This is really a rehash of my first observation. Interestingly, I compared the (debug) IL generated on my computer and they were exactly the same for both versions.
# Making != the exact opposite of ==
I believe it is safer to ensure that `!=` is the opposite of `==` by using `==` in the implementation of `!=`:
public static bool operator !=(Person person1, Person person2) => !(person1 == person2);
Any changes to `==` will automatically be reflected in `!=`.
Contributor guide
Assessment
This issue has not been assessed yet.