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.

  1. 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.)
  2. 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.
  3. There's absolutely no tests.
  4. 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.
  5. 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.
  6. 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.
  7. 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.
  8. 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

(tick)

 

DirectPosition2D

DirectPosition

(tick)

 

DirectPositionImpl

DirectPosition

(tick)

Arbitrary Dimensionality

GeometryImpl

Geometry

(tick)

getClosure() and getMaximalComplex() are stubs.

TransfiniteSetImpl

TransfiniteSet

(error)

All methods are stubs, but are implemented by GeometryImpl, the immediate (only) child of this class. Recommend removing this class.

EnvelopeImpl

Envelope

(tick)

 

BoundaryImpl

Boundary

(tick)

 

Overview of Geometric Primitive package

Class

GeoAPI Interface

Status

Comment

BearingImpl

Bearing

(warning)

Angles and direction are unrelated, angles have no associated units.

CurveImpl

Curve

(tick)

 

CurveBoundaryImpl

CurveBoundary

(tick)

 

CurveSegmentImpl

CurveSegment

(error)

Abstract class with no methods. Either consolidate subclass functionality or remove.

OrientableCurveImpl

OrientableCurve

(error)

Abstract class with no methods. Either consolidate subclass functionality or remove.

OrientablePrimitiveImpl

OrientablePrimitive

(error)

Abstract class with no methods. Either consolidate subclass functionality or remove.

OrientableSurfaceImpl

OrientableSurface

(error)

Abstract class with no methods. Either consolidate subclass functionality or remove.

PointImpl

Point

(warning)

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

(error)

Abstract class with no methods. Either consolidate subclass functionality or remove.

PrimitiveBoundaryImpl

PrimitiveBoundary

(error)

Abstract class with no methods. Either consolidate subclass functionality or remove.

PrimitiveFactoryImpl

PrimitiveFactory

(tick)

 

RingImpl

Ring

(warning)

Need to ensure that isSimple() and isCycle() are true.

ShellImpl

Shell

(error)

Stub, not used, remove.

SolidImpl

Solid

(error)

Stub, not used, remove.

SolidBoundaryImpl

SolidBoundary

(error)

Stub, not used, remove.

SurfaceImpl

Surface

(tick)

 

SurfaceBoundaryImpl

SurfaceBoundary

(tick)

 

SurfacePatchImpl

SurfacePatch

(tick)

 

Overview of Geometric Complex package

Class

GeoAPI Interface

Status

Comment

ComplexImpl

Complex

(tick)

Candidate for a "Notifying Set"

ComplexBoundaryImpl

ComplexBoundary

(tick)

 

CompositeImpl

Composite

(warning)

Should be an abstract class to prevent instantiation.

CompositeCurveImpl

CompositeCurve

(tick)

 

CompositeSurfaceImpl

CompositeSurface

(tick)

 

Overview of Coordinate Geometry package

Class

GeoAPI Interface

Status

Comment

ArcStringImpl

ArcString

(error)

Everything is stubbed out.

ArcImpl

Arc

(error)

Everything is stubbed out.

CircleImpl

Circle

(error)

Adds nothing to ArcImpl, it's parent...

ArcStringByBulgeImpl

ArcStringByBulge

(error)

Everything is stubbed out.

ArcByBulgeImpl

ArcByBulge

(error)

Adds nothing to ArcStringByBulge, it's parent...

ConicImpl

Conic

(error)

Everything is stubbed out.

GenericCurveImpl

GenericCurve

(tick)

Manages caching of JTS peer object.

GenericSurfaceImpl

GenericSurface

(error)

Abstract class with no methods. Either consolidate subclass functionality or remove.

GeodesicStringImpl

GeodesicString

(error)

Everything is stubbed out.

GeodesicImpl

Geodesic

(error)

Adds nothing to GeodesicStringImpl, it's parent...

GeometryFactoryImpl

GeometryFactory

(warning)

Some stubs could be fleshed out easily: e.g. LineSegment...

LineSegmentImpl

LineSegment

(tick)

 

LineStringImpl

LineString

(tick)

 

ParamForPointImpl

ParamForPoint

(error)

Everything is stubbed out.

PointArrayImpl

PointArray

(tick)

Likely to need efficiency improvements for large arrays...

PointGridImpl

PointGrid

(tick)

Suggest using java.awt.image.Raster for efficiency improvement (if/when needed).

PolygonImpl

Polygon

(tick)

 

PolyhedralSurfaceImpl

PolyhedralSurface

(warning)

Class does not constrain surface patches to be polygons. Needs to move from primitives to coordinate geometry package.

PositionImpl

Position

(tick)

This is just another name for "DirectPosition". Interface is superfluous.

Overview of Geometric Aggregate Package

Class

GeoAPI Interface

Status

Comment

AggregateImpl

Aggregate

(tick)

Candidate for "Notifying Set" usage.

MultiPointImpl

MultiPoint

(tick)

Neither GeoAPI nor this class define the derived "position" attribute.

MultiPrimitiveImpl

MultiPrimitive

(error)

Abstract class with no methods. Either consolidate subclass functionality or remove.