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

Make Geometry Tuple and Pattern Matching Friendly #735

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

mehrandvd
Copy link
Contributor

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository.
  • I have provided test coverage for my change (where applicable)

Description

Leveraging the latest features in C#, we can define Point, LineString, and LinearRing, variables in a more streamlined manner. I recommend the incorporation of implicit conversion operators and deconstructors to enhance usability and readability.

I've already added these features to the Coordinate family classes, and it's been merged by this PR: #733

With a well-defined implicit conversion operator, we can instantiate geometry objects as follows:

Point p1 = (1, 2);
Point p2 = (1, 2, 3);
LineString line = new[] {(1d, 2d), (3d, 4d)};
LinearRing ring = new[] { (1d, 2d), (3d, 4d), (1, 2) };

Similarly, by implementing an effective Deconstructor, we can extract values from our geometry objects like so:

var (x, y) = p1;
// or
var (x, _) = p1;
// or
var (x, y, z) = p1;
// or
var (x, y, _) = p1;

This approach not only simplifies the code but also enhances its readability and maintainability.

Pattern Matching

The interesting part is the pattern-matching capabilities that would be enabled:

if ( p is (1, 2, 3) ) { }
if ( p is (1, 2, _) ) { }
if ( p is (1, > 1, >= 3) ) { }
if ( p is (_, >= 2, _) ) { }

// and pattern-matching in switch expressions:

bool isValid = p switch
{
    (1, >1, >=3) => true,
    (_, >=2, _) => true,
    _ => false
};

@airbreather
Copy link
Member

I don't like the idea of making it easier to create new geometry instances without using a GeometryFactory.

@mehrandvd
Copy link
Contributor Author

@airbreather What about the pattern-matching capabilities?

@mehrandvd
Copy link
Contributor Author

@airbreather Just so you know, we have some constructors that don't require a GeometryFactory. For example, we have a Point constructor that just needs x and y coordinates. I've done my best to honor the existing constructors while adding implicit conversion operators that mirror them.

@airbreather
Copy link
Member

@airbreather What about the pattern-matching capabilities?

I am happy with the Deconstruct for Point. The Deconstruct for LineString seems fine too, although I'm less sure of how useful it will be in practice.

@airbreather Just so you know, we have some constructors that don't require a GeometryFactory. For example, we have a Point constructor that just needs x and y coordinates. I've done my best to honor the existing constructors while adding implicit conversion operators that mirror them.

Yes, I am aware. Such constructors have existed since the very first version. My hesitation is that I don't want to make it easier to create Geometry objects without supplying a GeometryFactory, which seems to be part of this PR's goal.

Arguably (and I may be the only one who thinks this) these constructors should be marked obsolete. JTS has been careful enough not to add anything like them: the latest version of Point.java has only two constructors, and the one (@deprecated) one without a GeometryFactory parameter accepts a PrecisionModel and a SRID, so it's not the same.

And even in JTS, where the geometry constructors always seem to accept GeometryFactory objects (and so they don't have the problems that I worry about with the extra ones found in NTS), you will still sometimes see messages like in locationtech/jts#651 (comment) which discourage using the constructors directly.

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

Successfully merging this pull request may close these issues.

None yet

2 participants