GeoTools : Acceptance Review of JTS Wrapper

Created by Bryce Nordgren, last modified by Adrian Custer on Jul 12, 2006

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.

The primary findings of this review

Icon

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:

The representation of geometric elements is carried out using 19107 concepts and data structures.

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

Status

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

Status

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

Status

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

Status

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.

Attachments:

Comments:

A few very basic (mostly DirectPosition and Envelope) geometry interfaces were implemented in the referencing module, because it needed them. Do we have a duplication between the following classes?

JTS Geometry

Referencing module

DirectPosition1D

DirectPosition1D

DirectPosition2D

DirectPosition2D

DirectPositionImpl

GeneralDirectPosition

EnvelopeImpl

GeneralEnvelope or ReferencedEnvelope

Note: I prefer GeneralDirectPosition rather than DirectPositionImpl since the General prefix suggest that this DirectPosition is about arbitrary dimension.

Generally speaking, I would vote for something else than the Impl suffix. What about JTS (prefix or suffix) as an indication that those implementations are JTS wrappers?

Posted by desruisseaux at Jun 04, 2006 18:50

I believe that these would indeed be duplicates. Is your usage such that you can manipulate just GeoAPI interfaces or do you need to reference an implementation? If we eliminate your implementation, then referencing and geometry have a codependency, which may not be all that bad...

As to naming: no one should ever see the names. These are created with a GeometryFactory. We can change the names however you'd like, though.