Expose ordinal constants and use instead of reflection

[bjornharrtell]

Primary goal of this PR is to avoid use of reflection in GeometryExtracter. Keeps existing API as deprecated.

The rename of SORTINDEX_* to ORDINAL_* is a suggestion from previous discussion.

[bjornharrtell]

Investigating test failure...

[bjornharrtell]

Problem is that the reflection logic will also extract LinearRings when LineString class is given.

[dr-jts]

Since the use of the numeric code is expanding beyond the Geometry class internals, I think a better name would be TYPECODE_XXX. This echoes the current Geometry type concept.

Or perhaps the Geometry type should be used for GeometryExtracter? This would require exposing them as named constants, which is no bad thing.

[bjornharrtell]

Agreed, I'll sleep on it and get back with something like one of your suggestions whatever seems nicest.

[bjornharrtell]

Opted to rename to TYPECODE and protect, and make GeometryType public (+ named constants) and use that for the GeometryExtractor.

[dr-jts]

Not sure about the presence of isLinear here. Wouldn't that mean that linear geometries are always added no matter what the requested type?

[bjornharrtell]

The original logic extracted LinearRings when the clz was set to LineString, which I didn't expect at first but I suppose that's how isAssignableFrom works. So that's why I added this exception to the rule. But perhaps it's the name isLinear which isn't right?

[dr-jts]

Ah, I understand the logic now. Wasn't clear because the logic for linear is split between that function and the isLinear one.

How about this instead?

protected static boolean isOfType(Geometry geom, int geometryType) {
  if (geom.getGeometryType() == geometryType) return true;
  if (geometryType == Geometry.TYPENAME_LINESTRING 
  	&& geom.getGeometryType() == Geometry.TYPENAME_LINEARRING && ) return true;
  return false;
}

public void filter(Geometry geom) {
  if (geometryType == null || isOfType(geom, geometryType))
    comps.add(geom);
}

[bjornharrtell]

Ah yes, much clearer - taken. :)

[bjornharrtell]

I see now I missed some JavaDocs updating. Will fix.

[bjornharrtell]

Should be all done now, hopefully.

[dr-jts]

Ideally GeometryExtracter should have a unit test too... :) (That would surface the subtle logic about linear handling).

Feel like giving this a shot? Or I can probably add one when this is merged.

[bjornharrtell]

Sure I'll give that a shot. :) But time is up for today.

[bjornharrtell]

@dr-jts testcase added. Quite simplistic but does the intended verification I hope.

[dr-jts]

@bjornharrtell This looks good. If it's complete I will merge.

[jnh5y]

@bjornharrtell are you finished with this PR? Looks like Martin might be looking confirmation before merging.

[bjornharrtell]

Good to go :)

[bjornharrtell]

@dr-jts, gentle reminder that this is ready to merge.

[jodygarnett]

collecting PRs, @bjornharrtell + @dr-jts ?

[bjornharrtell]

I believe this is ready.

[bjornharrtell]

Squashed and rebased on master, all tests pass. Still ready for merge AFAIK. :)

[bjornharrtell]

@dr-jts and @jodygarnett I've just rebased this on master - I still think it's a general improvement and a necessity for JSTS.