Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement GetHashCode and Equals in terms of the type hierarchy #274

Open
rianjs opened this issue Apr 28, 2017 · 4 comments
Open

Implement GetHashCode and Equals in terms of the type hierarchy #274

rianjs opened this issue Apr 28, 2017 · 4 comments

Comments

@rianjs
Copy link
Collaborator

rianjs commented Apr 28, 2017

Extending some of the ideas from #271, calendar components should delegate to their parents the hashing and equality-checking for the properties that their parent types own, and simplify down to the properties that the child type owns.

For example, Event does its own thing, even though there is a type hierarchy above it that it completely ignores. It should probably use the type hierarchy instead. This way, bugfixes and feature enhancements will propagate to the other child types as well.

This may require spending some time understanding the relationship between UniqueComponent and RecurringComponent, and moving some of the hashing and equality logic to other places.

@rianjs
Copy link
Collaborator Author

rianjs commented May 24, 2018

C.f. #393

@axunonb
Copy link
Collaborator

axunonb commented Jan 19, 2025

Here we have classes overriding Equals().

The equality comparison is quite a burden for the bigger classes. When it comes to GetHashCode there are many of them using mutable fields, which limits the usability in hash-based collections anyway. Often it's tricky to decide which fields should be included in the comparison, and which should not.

For which of the classes do we see a practical advantage to override Equals()?
I have the low-level classes in mind as candidates.

@minichma @rianjs What do you think?

Code File Line Column

  • public override bool Equals(object obj) | Calendar.cs 93 4
  • public override bool Equals(object obj) | CalendarCollection.cs 95 4
  • public override bool Equals(object? obj) | CalendarComponents\CalendarEvent.cs 310 4
  • public override bool Equals(object obj) | CalendarComponents\Journal.cs 39 4
  • public override bool Equals(object obj) | CalendarComponents\RecurringComponent.cs 221 4
  • public override bool Equals(object obj) | CalendarComponents\UniqueComponent.cs 98 4
  • public override bool Equals(object obj) | CalendarComponents\VTimeZone.cs 368 4
  • public override bool Equals(object? obj) | CalendarObject.cs 59 4
  • public override bool Equals(object obj) | DataTypes\AlarmOccurrence.cs 54 4
  • public override bool Equals(object obj) | DataTypes\Attachment.cs 104 4
  • public override bool Equals(object obj) | DataTypes\Attendee.cs 282 4
  • public override bool Equals(object? obj) | DataTypes\CalDateTime.cs 241 4
  • public override bool Equals(object obj) | DataTypes\GeographicLocation.cs 55 4
  • public override bool Equals(object obj) | DataTypes\Occurrence.cs 30 4
  • public override bool Equals(object? obj) | DataTypes\Organizer.cs 93 4
  • public override bool Equals(object? obj) | DataTypes\Period.cs 122 4
  • public override bool Equals(object? obj) | DataTypes\PeriodList.cs 90 4
  • public override bool Equals(object? obj) | DataTypes\RecurrencePattern.cs 164 4
  • public override bool Equals(object obj) | DataTypes\RequestStatus.cs 72 4
  • public override bool Equals(object obj) | DataTypes\StatusCode.cs 67 4
  • public override bool Equals(object obj) | DataTypes\Trigger.cs 101 4
  • public override bool Equals(object obj) | DataTypes\UTCOffset.cs 48 4
  • public override bool Equals(object obj) | DataTypes\WeekDay.cs 43 4
  • public override bool Equals(object obj) | VTimeZoneInfo.cs 50 4

@minichma
Copy link
Collaborator

@axunonb Agree that Equals doesn't make too much sense in many classes. Agree to your list. Maybe double-check which types we use HashSet, Dictionary, OrderBy, ... (ask CoPilot which others) on (i.e. those that use Equals/GetHashCode under the hoods). Sometimes it might even be better to have an external Comarator or EqualityComparator for a specific use than implementing Equals where there is no single natural equality.

@axunonb
Copy link
Collaborator

axunonb commented Jan 21, 2025

Here is the revised list after checking manually whether Equals or GetHashCode are in use, and how.

  • public override bool Equals(object obj) | Calendar.cs | equality tests
  • public override bool Equals(object obj) | CalendarCollection.cs | equality tests
  • public override bool Equals(object? obj) | CalendarComponents\CalendarEvent.cs | equality tests
  • public override bool Equals(object obj) | CalendarComponents\Journal.cs | equality tests
  • public override bool Equals(object obj) | CalendarComponents\RecurringComponent.cs | equality tests
  • public override bool Equals(object obj) | CalendarComponents\UniqueComponent.cs | not used at all
  • public override bool Equals(object obj) | CalendarComponents\VTimeZone.cs | equality tests
  • public override bool Equals(object? obj) | CalendarObject.cs | IN USE
  • public override bool Equals(object obj) | DataTypes\AlarmOccurrence.cs | not used at all
  • public override bool Equals(object obj) | DataTypes\Attachment.cs | equality tests
  • public override bool Equals(object obj) | DataTypes\Attendee.cs | equality tests
  • public override bool Equals(object? obj) | DataTypes\CalDateTime.cs | IN USE
  • public override bool Equals(object obj) | DataTypes\GeographicLocation.cs | equality tests
  • public override bool Equals(object obj) | DataTypes\Occurrence.cs | IN USE
  • public override bool Equals(object? obj) | DataTypes\Organizer.cs | not used at all
  • public override bool Equals(object? obj) | DataTypes\Period.cs | IN USE
  • public override bool Equals(object? obj) | DataTypes\PeriodList.cs | equality tests
  • public override bool Equals(object? obj) | DataTypes\RecurrencePattern.cs | via CalendarEvent, equality tests
  • public override bool Equals(object obj) | DataTypes\RequestStatus.cs | not used at all
  • public override bool Equals(object obj) | DataTypes\StatusCode.cs | not used at all
  • public override bool Equals(object obj) | DataTypes\Trigger.cs | equality tests
  • public override bool Equals(object obj) | DataTypes\UTCOffset.cs | via Calendar, equality tests
  • public override bool Equals(object obj) | DataTypes\WeekDay.cs | via RecurrencePattern and Attachment, equality tests
  • public override bool Equals(object obj) | VTimeZoneInfo.cs | not used at all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants