dotnet / dotnet/dotnet-api-docs

Improvements to IEquatable<T>.Equals example

Offen
#1,995 0 Kommentare 3 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
area-System.Runtime Pri3 untriaged
Vorherrschende Sprache
C#
Sterne
950
Forks
1.7k
Ø Merge
2 T. 19 Std.
Gemergte PRs (30 T.)
52

Beschreibung

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 `!=`.

Beitragsleitfaden

Beitragsleitfaden öffnen

Rechercherichtung

Öffne das verlinkte snippets/csharp/VS_Snippets_CLR_System/system.GenericIEquatable.Equals/cs/EqualsEx2.cs-Beispiel und vergleiche dessen Equals-, ==- und !=-Implementierungen mit den fünf Beobachtungen im Issue. Hole zuerst die Zustimmung der Maintainer dazu ein, welche subjektiven Änderungen gewünscht sind; erledigt ist die Aufgabe, wenn die akzeptierte Anleitung konsistent im Beispiel umgesetzt ist.

Vom Indexierungsmodell aus dem Issue-Text verfasst.

Bewertung

Tech-Stack
csharp
Bereich
documentation
Issue-Typ
Dokumentation
Schwierigkeit
5/5
Geschätzter Aufwand
Über eine Woche
Aktivitätsstatus
Veraltet
Klarheit
Größtenteils klar
Anfängerfreundlichkeit
35/100

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.