GeoAPI
  1. GeoAPI
  2. GEO-87

"Position" interface seems superfluous.

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.2-M1
    • Component/s: geometry
    • Labels:
      None
    • Number of attachments :
      0

      Description

      ISO 19107 defines a GM_Position as a union between a DirectPosition type, and a reference to a DirectPosition. However, in Java, all instance variables contain references to objects and it is impossible to "pass by value". This has resulted in a "...spatialschema.geometry.geometry.Position" interface which just maintains a reference to a DirectPosition.

      I don't think we'd be losing anything if we eliminated this interface and replaced all references to it with DirectPosition.

        Activity

        Hide
        Jody Garnett added a comment -

        In reviewing code it seems that Position is extended by DirectPosition and Point.

        This lets us use Points (ie a full Geometry) as part of our LineString definition ... it would not be good to lose this.

        Show
        Jody Garnett added a comment - In reviewing code it seems that Position is extended by DirectPosition and Point. This lets us use Points (ie a full Geometry) as part of our LineString definition ... it would not be good to lose this.
        Hide
        Bryce Nordgren added a comment -

        It appears that my original statement of the problem was in error. A GM_Position should be a union between a DirectPosition (which it owns) or a Point (which it maintains a reference to.)

        So the problem becomes "how do we correctly implement a <<Union>>", which may have a fairly straightforward answer. Why not have two methods, either one of which may return null (but one must return a value):

        + public DirectPosition getDirect() ;
        + public Point getIndirect() ;

        This would seem to be consistent with the UML in Figure 14 of OGC Topic 1 and the text in Clause 6.4.5.

        Neither Point nor DirectPosition should extend Position unless this is how we intend to implement <<Unions>>. Implementing <<Union>> with a type heirarchy seems confusing to me because it produces parent-child relationships where none should exist.

        Looking at this code again, I think it would be very hard to use this interface! "Cast the object if it is a Point or call getPosition() if the object is a DirectPosition?" You're just iterating over a collection of Positions for the most part. You're not supposed to write code which assumes that the Position is one representation or the other. If we remove the inheritance and put in the two methods above, we have an even handed way of checking which representation is present.

        Show
        Bryce Nordgren added a comment - It appears that my original statement of the problem was in error. A GM_Position should be a union between a DirectPosition (which it owns) or a Point (which it maintains a reference to.) So the problem becomes "how do we correctly implement a <<Union>>", which may have a fairly straightforward answer. Why not have two methods, either one of which may return null (but one must return a value): + public DirectPosition getDirect() ; + public Point getIndirect() ; This would seem to be consistent with the UML in Figure 14 of OGC Topic 1 and the text in Clause 6.4.5. Neither Point nor DirectPosition should extend Position unless this is how we intend to implement <<Unions>>. Implementing <<Union>> with a type heirarchy seems confusing to me because it produces parent-child relationships where none should exist. Looking at this code again, I think it would be very hard to use this interface! "Cast the object if it is a Point or call getPosition() if the object is a DirectPosition?" You're just iterating over a collection of Positions for the most part. You're not supposed to write code which assumes that the Position is one representation or the other. If we remove the inheritance and put in the two methods above, we have an even handed way of checking which representation is present.
        Hide
        Jody Garnett added a comment -

        Ah - thanks for hunting down a question I can answer. Lets look at a couple of ways...

        Aside: I always capture adapting to different types with with a "to" methods (the adapter pattern is the OO way to do a a union).

        I would also suck out the useful methods so that client code does not have to notice the split unless they want to modify something.

        Here is what that looks like

        interface Position  {
           // may be this or direct position hosted by a referenced point
           DirectPosition toDirectPosition();
           
           // Common methods
           CoordinateReferenceSystem getCoordinateReferenceSystem();
           double[] getCoordinates();
        }
        intreface DirectPosition extends Position {
          DirectPosition toDirectPosition();   // this
        
           int getDimension();
           double getOrdinate( int dimension );
           void setOrdinate( int dimension, double value );
        }
        interface PointPosition extends Position {
           DirectPosition toDirectPosition(); // Access toPoint().getPosition();
        
           Point toPoint();   // explicitly model the reference
        }
        
        Show
        Jody Garnett added a comment - Ah - thanks for hunting down a question I can answer. Lets look at a couple of ways... Aside: I always capture adapting to different types with with a "to" methods (the adapter pattern is the OO way to do a a union). I would also suck out the useful methods so that client code does not have to notice the split unless they want to modify something. Here is what that looks like interface Position { // may be this or direct position hosted by a referenced point DirectPosition toDirectPosition(); // Common methods CoordinateReferenceSystem getCoordinateReferenceSystem(); double [] getCoordinates(); } intreface DirectPosition extends Position { DirectPosition toDirectPosition(); // this int getDimension(); double getOrdinate( int dimension ); void setOrdinate( int dimension, double value ); } interface PointPosition extends Position { DirectPosition toDirectPosition(); // Access toPoint().getPosition(); Point toPoint(); // explicitly model the reference }
        Hide
        Martin Desruisseaux added a comment -

        Actually Point already has a getPosition() method which returns a DirectPosition. Admitly the getPosition() name is confusing and would be better named as getDirectPosition(). So proposed action:

        • Rename getPosition() as getDirectPosition().
        • Make it clear in the javadoc that the above returns this in the DirectPosition case, and behave as documented by ISO in the Point case.

        So users don't need to perform special check. They can just invoke getDirectPosition() directly.

        Show
        Martin Desruisseaux added a comment - Actually Point already has a getPosition() method which returns a DirectPosition . Admitly the getPosition() name is confusing and would be better named as getDirectPosition() . So proposed action: Rename getPosition() as getDirectPosition() . Make it clear in the javadoc that the above returns this in the DirectPosition case, and behave as documented by ISO in the Point case. So users don't need to perform special check. They can just invoke getDirectPosition() directly.
        Martin Desruisseaux made changes -
        Field Original Value New Value
        Assignee Martin Desruisseaux [ desruisseaux ]
        Martin Desruisseaux made changes -
        Fix Version/s 2.2 [ 12386 ]
        Hide
        Martin Desruisseaux added a comment -

        Renamed getPosition() as getDirectPosition() in the hope to avoid confusion. Except for this renaming, the Position interface is left unchanged since its purpose is to mimic C/C++ enum functionality.

        Show
        Martin Desruisseaux added a comment - Renamed getPosition() as getDirectPosition() in the hope to avoid confusion. Except for this renaming, the Position interface is left unchanged since its purpose is to mimic C/C++ enum functionality.
        Martin Desruisseaux made changes -
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Closed [ 6 ]
        Martin Desruisseaux made changes -
        Status Closed [ 6 ] Reopened [ 4 ]
        Resolution Fixed [ 1 ]
        Martin Desruisseaux made changes -
        Status Reopened [ 4 ] Closed [ 6 ]
        Fix Version/s 2.2-M1 [ 14472 ]
        Fix Version/s 2.2 [ 12386 ]
        Resolution Fixed [ 1 ]

          People

          • Assignee:
            Martin Desruisseaux
            Reporter:
            Bryce Nordgren
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: