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

Remove redundant interface members #67

Closed
airbreather opened this issue Mar 3, 2019 · 2 comments
Closed

Remove redundant interface members #67

airbreather opened this issue Mar 3, 2019 · 2 comments
Labels
breaking change Doing this would break either source or binary compatibility with existing versions.

Comments

@airbreather
Copy link
Member

airbreather commented Mar 3, 2019

Every member defined on an interface represents an added cost to whoever is ultimately going to implement it. Some of our interfaces have gotten way out of hand, and #30* seeks to help with that by segregating the interfaces more so that the implementer doesn't have to account for everything under the sun.

  • *looking at just the original issue, not what the comments turned it into

However, we could get some "quick wins" towards usability by removing some members that are outright redundant with other members already on the same interface (adding extension methods in their place where appropriate to avoid harming usability).

I propose that we try to simplify as much of this list as we reasonably can...

ICoordinateBuffer

AddCoordinate(...) is redundant with InsertCoordinate(Count, ...), but can we just get rid of this interface?

ICoordinateSequence

Ordinate / Ordinates and friends... that's a whole other topic of its own.

GetCoordinateCopy(i) is equivalent to GetCoordinate(i, CreateCoordinate()).

ToCoordinateArray() is a bunch of GetCoordinateCopy(i) calls to initialize new Coordinate[Count].

ExpandEnvelope(env) is a bunch of calls to env.ExpandToInclude(GetX(i), GetY(i)).

ICoordinateSequenceFactory

Again, leaving out Ordinate / Ordinates and friends.

ICoordinateSequence Create(int size, int dimension, int measures) is all that implementations should need to care about implementing.

"Create it as a copy of this other thing" can have a major impact on performance for pretty much every implementation (there's almost always a better way to copy from an instance of the same type), however, I think we can satisfy that need almost as well with a new "default interface method"*:

public interface ICoordinateSequence
{
    /* ... */

    void CopyFrom(ICoordinateSequence other, int offset, int count, IEnumerable<Ordinate> ordinatesToCopy)
    {
        // not writing parameter validation here
        var ordinatesToCopyArray = ordinatesToCopy.ToArray();
        for (int i = 0; i < count; i++)
        {
            foreach (var ordinate in ordinatesToCopyArray)
            {
                this.SetOrdinate(i, ordinate, other.GetOrdinate(i + offset, ordinate));
            }
        }
    }
}

*I know that we can't do default interface methods properly, but there's a way to cheat at it that's almost as good... instead of a true "default interface method", we can use an extension method with a helper interface... to give an idea, an interface with a "default method" that we wish we could write like this:

public interface ISampleInterface
{
    void DoTheThing() => Console.WriteLine("I am doing the thing, but not very well");
}

could be delivered like this:

public interface ISampleInterface
{
    // DoTheThing is *NOT* here.
}

public interface IDoTheThingBetter
{
    void DoTheThing();
}

public static class SampleInterfaceExtensions
{
    public static void DoTheThing(this ISampleInterface @this)
    {
        if (@this is IDoTheThingBetter thingDoer)
        {
            // "overridden" implementation
            thingDoer.DoTheThing();
            return;
        }

        // default implementation
        Console.WriteLine("I am doing the thing, but not very well");
    }
}

ICurve

IsClosed is the same as checking whether StartPoint and EndPoint are equal.

IsRing is apparently IsClosed && IsSimple, according to its single implementation that isn't called by anyone I can discover right now.

IGeometry

Strict redundancies

  1. SRID is Factory.SRID.
  2. GeometryType looks like GetType().Name???
  3. Envelope is Factory.ToGeometry(EnvelopeInternal).
  4. Normalized() is Copy().Normalize() + return the normalized copy.
  5. EqualsExact(g) is EqualsExact(g, 0).
  6. EqualsNormalized(g) is Normalized().EqualsExact(g.Normalized()).
  7. PointOnSurface is InteriorPoint.
  8. x.Within(y) is y.Contains(x).
  9. x.CoveredBy(y) is y.Covers(x).
  10. x.Disjoint(y) is !x.Intersects(y).
  11. Relate(g, somePattern) is Relate(g).Matches(somePattern).
  12. Buffer(double, IBufferParameters) is the only Buffer method we need.
    • though I'd kinda like IBufferParameters not to be an interface...

Can be implemented via a call to Apply(...)

  1. int NumGeometries
  2. int NumPoints
  3. Coordinate Coordinate
  4. Coordinate[] Coordinates
  5. double[] GetOrdinates(Ordinate)
  6. Dimension Dimension (but OgcGeometryType will often be enough)
  7. IGeometry Envelope
  8. Envelope EnvelopeInternal
  9. bool IsEmpty

Can be implemented via Relate(g), though there's often a decent way to short-circuit.

  1. Contains / Within
  2. Covers / CoveredBy
  3. Intersects / Disjoint
  4. Touches
  5. Crosses
  6. Overlaps
  7. EqualsTopologically

IGeometryCollection

  1. int Count is NumGeometries
  2. this[int] is GetGeometryN

IGeometry[] Geometries also could be this.ToArray() if we're OK with no longer defining it as a view of the underlying data... not exactly a discussion for here though.

IGeometryFactory

These methods are variants of other methods invoke those other methods with some sort of "empty":

  1. CreatePoint()
  2. CreateLineString()
  3. CreateLinearRing()
  4. CreatePolygon()
  5. CreatePolygon(ILinearRing)
  6. CreateMultiPoint()
  7. CreateMultiLineString()
  8. CreateMultiPolygon()
  9. CreateGeometryCollection()

These methods are variants of other ICoordinateSequence methods via CoordinateSequenceFactory:

  1. CreatePoint(Coordinate)
  2. CreateLineString(Coordinate[])
  3. CreateLinearRing(Coordinate[])
  4. CreatePolygon(Coordinate[])
  5. CreateMultiPointFromCoords(Coordinate[])

Finally, CreatePolygon(ICoordinateSequence) is CreatePolygon(CreateLineString(seq))

ILineString

  1. GetCoordinateN(i) is CoordinateSequence.GetCoordinate(i)
  2. GetPointN(i) is Factory.CreatePoint(GetCoordinateN(i))

IMultiCurve

bool IsClosed is !IsEmpty && Cast<ICurve>().All(g => g.IsClosed)

IPoint

X is CoordinateSequence.GetX(0). Same for Y, Z, M.

IPolygon

Same idea as in IGeometryCollection, we only need either the array or the Num +

  1. ExteriorRing is Shell.
  2. ILineString[] InteriorRings is Holes.ToArray<ILineString>()
  3. int NumInteriorRings / GetInteriorRingN(int) come from Holes, or vice-versa if we don't want to keep defining Holes as a view of the underlying data.

IPrecisionModel

bool IsFloating, int MaximumSignificantDigits, and void MakePrecise(Coordinate) are all defined to be the same as their NTS implementations.

IGeometryServices

CreatePrecisionModel(IPrecisionModel) is one of the two other overloads depending on the type.

All CreateGeometryFactory overloads are equivalent to CreateGeometryFactory(IPrecisionModel, int, ICoordinateSequenceFactory), filling in the defaults from properties where missing.

@airbreather airbreather added the breaking change Doing this would break either source or binary compatibility with existing versions. label Mar 3, 2019
@airbreather airbreather modified the milestones: v2.0, v2.0 Candidate Mar 3, 2019
@FObermaier
Copy link
Member

FObermaier commented Mar 13, 2019

Side note: I personally like the Ordinate and Ordiantes enums more than HasZ and HasM.

Interface Annotation
ICoordinateBuffer Is used here. Used to be in a NetTopologySuite.IO library for reuse in Shapefile IO, but never made it that far.
ICoordinateSequence The comment to ToCoordinateArray() is partially wrong. For CoordinateArraySequence the internal array is returned without copying anything

Additionally, please see #68 (comment)

airbreather added a commit to NetTopologySuite/NetTopologySuite that referenced this issue Apr 10, 2019
Most notably, IGeometry is no more.  Use Geometry instead.

Related work items:
- NetTopologySuite/GeoAPI#67
- NetTopologySuite/GeoAPI#68
@airbreather
Copy link
Member Author

We did #68 instead.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking change Doing this would break either source or binary compatibility with existing versions.
Projects
None yet
Development

No branches or pull requests

2 participants