Skip to content
This repository has been archived by the owner on Jun 22, 2023. It is now read-only.

IEquatable<T> Is A Bad Idea On Unsealed Types #29

Closed
airbreather opened this issue Aug 21, 2017 · 2 comments
Closed

IEquatable<T> Is A Bad Idea On Unsealed Types #29

airbreather opened this issue Aug 21, 2017 · 2 comments

Comments

@airbreather
Copy link
Member

airbreather commented Aug 21, 2017

IEquatable<T> exists for basically one purpose only: to avoid the performance-related overhead of casting + boxing/unboxing overhead of object when an instance of the type is stored in a hash-based collection. Without the concern of performance, bool Equals(object) would be sufficient. So if type Foo is expected to be used as the key in a hash-based collection, especially if it's a value type, it is sometimes recommended that Foo should implement IEquatable<Foo>.

I've never seen a use case where it is useful for an interface type IFoo to implement IEquatable<IFoo>, and every time I have seen this at work, it has been a mistake. Same goes for the NTS projects, it seems.

I'm pretty sure this actually makes GeoAPI's GeoAPI.Geometries.IGeometry differ semantically from JTS's org.locationtech.jts.geom.Geometry, because the latter just uses the Java equivalent to .NET's bool Equals(object obj), which uses boolean equalsExact(Geometry other), for all Java collections:

  • new Dictionary<ILineString, string>() will use bool Equals(object obj) to compare keys.
  • new Dictionary<Geometry, string>() will use bool Equals(object obj) to compare keys.
  • new Dictionary<IGeometry, string>() will use bool Equals(IGeometry other) to compare keys.

It's possible to "fix" IEquatable<T> and keep it around by taking some breaking changes, but the result would be expensive to implementers and confusing to maintainers and consumers alike. I think we should consider a breaking change to drop every IEquatable<T> from this repository, with one exception: Interval is actually recommended to implement this interface, because it's a struct type.

After, we should consider whether or not to add bool Equals(IGeometry geometry) itself to IGeometry.

  • I personally think we shouldn't have that on IGeometry because it already has both bool EqualsExact(IGeometry other) and bool EqualsTopologically(IGeometry other).
  • JTS javadoc mentions that its boolean equals(Geometry g) is only there "for backward compatibility reasons" now that equalsTopo exists.
  • This is tricky to do, ourselves, in such a way that the result is both backwards-compatible and faithful to JTS. Perhaps what we should do is keep bool Equals(IGeometry other) around, but mark it with [Obsolete(..., error: true)] to force consumers to explicitly say EqualsTopologically if that's what they want.

I have similar thoughts about IComparable<T>, but that's a trickier subject because we inherited some of that messiness from JTS, so I'm not sure what I want to propose that we do about that one yet. It'll be a separate issue, if anything.

@FObermaier
Copy link
Member

I support the removal of IEquatable<IGeometry>, even though it has been there from the very beginning.
IComparable<IGeometry> deserves a seperate issue.

@airbreather
Copy link
Member Author

Note: the concerns here may intersect somewhat with locationtech/jts#159.

airbreather added a commit that referenced this issue Oct 13, 2017
airbreather added a commit to NetTopologySuite/NetTopologySuite that referenced this issue Oct 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants