Skip to content

Conversation

@otan
Copy link
Collaborator

@otan otan commented May 18, 2020

  • Add Empty() to the T interface, which is defined on all geom.T types.
  • Add interface assertions for all the types, makes unimplemented
    interface errors easier to read at compile time.

* Add Empty() to the T interface, which is defined on all geom.T types.
* Add interface assertions for all the types, makes unimplemented
interface errors easier to read at compile time.
@twpayne
Copy link
Owner

twpayne commented May 18, 2020

Thanks for the contribution!

What's the reason for adding Empty() to the geom.T interface?

Unfortunately GeometryCollections behave slightly differently to normal geometry types like Point and LineString, and GeometryCollection's Empty() method has subtly different semantics. Specifically, a normal geometry is empty if it has no points, whereas a GeometryCollection is empty if it has no geometries in it, and consequently a GeometryCollection containing several empty geometries is not itself empty, even if it has no points.

There are a couple of other methods (Area() and Length()) that are implemented on all geometry types except GeometryCollection and there are existence tests for them at https://github.com/twpayne/go-geom/blob/master/geom_test.go#L8-L31.

The reason that Empty, Area, and Length are not part of geom.T is because of the special case handling needed for GeometryCollection. Basically, to interpret the result of Empty() correctly, you have to do a typecast to see if your geom.T is a GeometryCollection or not, and if you've done a typecast then you have a pointer to the underlying type and call Empty() (or Area() or Length()) on that directly.

As a stylistic nit, for adding interface satisfaction I would prefer them to be in a _test.go file (e.g. geom_test.go) and you can then use the form

var _ geom.T = &Point{}

as you don't care about the object being created. This is slightly more concise than (*Point)(nil).

GeometryCollections semantics are so different to the "normal" geometry types that it is really tricky to implement a generic interface that handles both. On top of that, I've never actually seen a GeometryCollection used in a real GIS app: I have not yet seen a need for a value that is a mixed bag of different geometries of mixed layouts. It would be so much easier if the OGC had not included GeometryCollections in the spec.

Anyhow, what's the underlying need to add Empty() to the geom.T interface?

@otan
Copy link
Collaborator Author

otan commented May 18, 2020

Anyhow, what's the underlying need to add Empty() to the geom.T interface?

I find myself repeating code a lot to handle the same Empty() case, I'm guessing #156 is really what I'm looking for.

Take a look at:

Its more or less the same checks (#156 covers this fully) but copy pasting it seems a little cumbersome.

As a stylistic nit, for adding interface satisfaction I would prefer them to be in a _test.go file (e.g. geom_test.go) and you can then use the form

works either way for me! I think it's neater near the definition. I can do that if we decide to 🚢 it - do we want to ship #156 instead, perhaps?

@twpayne
Copy link
Owner

twpayne commented May 18, 2020

Let's go with #156 :)

@twpayne twpayne closed this May 18, 2020
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.

2 participants