Acceptance Review of JTS Wrapper

Executive Summary

This page details the findings of the pre-acceptance code review for the GeoTools ISO 19107 implementation backed by JTS wrappers. This code was developed by the employees of Sys Technologies, Inc. and refactored into the GeoTools namespace by Colin Combe.

Info

title	The primary findings of this review

The implementation correctly implements the GeoAPI interfaces for those cases where an implementation is provided. In some cases, stubbed out methods return inappropriate values (e.g., null values instead of an empty collection, or an orientation value of 0 instead of +/-1.)
Protecting the defining components of the geometry objects would be my number one concern. Certain geometric objects maintain modifiable Sets or Lists of points, curves, etc. The method of altering these geometries is to obtain a reference to these backing Lists or Sets, then add or delete elements. A "NotifyingArrayList" implementation is provided which invalidates the cached JTS equivilant object. I would like to see an entire family of NotifyingCollections used exhaustively throughout the entire module to ensure that any and all modifications invalidate the cached JTS object.
There's absolutely no tests.
In some places, notably in the implementation of topological operations, GeoAPI interfaces are blindly cast to classes from this implementation. For instance, when computing the intersection, difference, etc. of geometries, the "other" GeoAPI geometry object is assumed to also be from this implementation. It may very well be valid to not support inter-implementation comparisons, but perhaps it is more appropriate to throw an UnsupportedOperationException. I would recommend letting this issue go for now and revisiting it in the future.
This implementation does no checking to ensure that the computational geometry operations are only performed within the context of coordinate systems known to work with JTS. I am unsure whether fixing this would help or hinder users, as they may very well want "approximately correct" results in a lat/lon reference frame.
ISO 19107 supports the notion that geometries may contain other geometries. This implementation supports knowing about the geometries contained by the current geometry, but does not support knowing about which geometry might contain the current one.
This implementation is representationally inefficient when JTS operations must be performed. It will hit a memory barrier before direct utilization of JTS would:
1. The representation of geometric elements is carried out using 19107 concepts and data structures.
2. When a JTS specific operation must be performed, equivilent representations with JTS data structures are calculated.
This implementation (and indeed, GeoAPI itself) contains no support for Topological complexes.

Detailed review of this code may be found in the database attached to this page. This database is a "mini-issue-tracker" implemented in OpenOffice 2.0 Base. This database contains information about which classes are present, which interfaces are implemented, and which classes or methods are "stubs". As of this moment, I have not implmented reports, but this may be achieved by the knowledgable user. The database itself is fully populated by observations.

Overview of Geometry Root package

Class	GeoAPI Interface	Comment
DirectPosition1D	DirectPosition
DirectPosition2D	DirectPosition
DirectPositionImpl	DirectPosition	Arbitrary Dimensionality
GeometryImpl	Geometry	`getClosure()` and `getMaximalComplex()` are stubs.
TransfiniteSetImpl	TransfiniteSet	All methods are stubs, but are implemented by GeometryImpl, the immediate (only) child of this class. Recommend removing this class.
EnvelopeImpl	Envelope
BoundaryImpl	Boundary

Overview of Geometric Primitive package

Class	GeoAPI Interface	Comment
BearingImpl	Bearing	Angles and direction are unrelated, angles have no associated units.
CurveImpl	Curve
CurveBoundaryImpl	CurveBoundary
CurveSegmentImpl	CurveSegment	Abstract class with no methods. Either consolidate subclass functionality or remove.
OrientableCurveImpl	OrientableCurve	Abstract class with no methods. Either consolidate subclass functionality or remove.
OrientablePrimitiveImpl	OrientablePrimitive	Abstract class with no methods. Either consolidate subclass functionality or remove.
OrientableSurfaceImpl	OrientableSurface	Abstract class with no methods. Either consolidate subclass functionality or remove.
PointImpl	Point	Will not calculate bearing to another point; when setting location, attempts to convert to this Point's current CRS instead of adopting the new CRS. OK?
PrimitiveImpl	Primitive	Abstract class with no methods. Either consolidate subclass functionality or remove.
PrimitiveBoundaryImpl	PrimitiveBoundary	Abstract class with no methods. Either consolidate subclass functionality or remove.
PrimitiveFactoryImpl	PrimitiveFactory
RingImpl	Ring	Need to ensure that `isSimple()` and `isCycle()` are true.
ShellImpl	Shell	Stub, not used, remove.
SolidImpl	Solid	Stub, not used, remove.
SolidBoundaryImpl	SolidBoundary	Stub, not used, remove.
SurfaceImpl	Surface
SurfaceBoundaryImpl	SurfaceBoundary
SurfacePatchImpl	SurfacePatch

Overview of Geometric Complex package

Class	GeoAPI Interface	Comment
ComplexImpl	Complex	Candidate for a "Notifying Set"
ComplexBoundaryImpl	ComplexBoundary
CompositeImpl	Composite	Should be an abstract class to prevent instantiation.
CompositeCurveImpl	CompositeCurve
CompositeSurfaceImpl	CompositeSurface

Overview of Coordinate Geometry package

Class	GeoAPI Interface	Comment
ArcStringImpl	ArcString	Everything is stubbed out.
ArcImpl	Arc	Everything is stubbed out.
CircleImpl	Circle	Adds nothing to `ArcImpl`, it's parent...
ArcStringByBulgeImpl	ArcStringByBulge	Everything is stubbed out.
ArcByBulgeImpl	ArcByBulge	Adds nothing to `ArcStringByBulge`, it's parent...
ConicImpl	Conic	Everything is stubbed out.
GenericCurveImpl	GenericCurve	Manages caching of JTS peer object.
GenericSurfaceImpl	GenericSurface	Abstract class with no methods. Either consolidate subclass functionality or remove.
GeodesicStringImpl	GeodesicString	Everything is stubbed out.
GeodesicImpl	Geodesic	Adds nothing to `GeodesicStringImpl`, it's parent...
GeometryFactoryImpl	GeometryFactory	Some stubs could be fleshed out easily: e.g. LineSegment...
LineSegmentImpl	LineSegment
LineStringImpl	LineString
ParamForPointImpl	ParamForPoint	Everything is stubbed out.
PointArrayImpl	PointArray	Likely to need efficiency improvements for large arrays...
PointGridImpl	PointGrid	Suggest using `java.awt.image.Raster` for efficiency improvement (if/when needed).
PolygonImpl	Polygon
PolyhedralSurfaceImpl	PolyhedralSurface	Class does not constrain surface patches to be polygons. Needs to move from primitives to coordinate geometry package.
PositionImpl	Position	This is just another name for "DirectPosition". Interface is superfluous.

Overview of Geometric Aggregate Package

Class	GeoAPI Interface	Comment
AggregateImpl	Aggregate	Candidate for "Notifying Set" usage.
MultiPointImpl	MultiPoint	Neither GeoAPI nor this class define the derived "position" attribute.
MultiPrimitiveImpl	MultiPrimitive	Abstract class with no methods. Either consolidate subclass functionality or remove.