dotnet / dotnet/dotnet-api-docs

Improvements to IEquatable<T>.Equals example

Open
#1,995 0 comments 3 reactions 0 assignees View on GitHub
area-System.Runtime Pri3 untriaged
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.