Added by Bryce Nordgren, last edited by Adrian Custer on Jul 12, 2006  (view change)

Labels:

Enter labels to add to this page:
Wait Image 
Looking for a label? Just start typing.

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
  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  
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.
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 Status 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.

Comments  (Hide 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 Martin 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.

Posted by Bryce Nordgren at Jun 13, 2006 15:05