Mutable vs immutable data structures

The key differences observed between GeoAPI interfaces and GeoTools interfaces:

• GeoTools interfaces are often read-write (with set methods or direct list access), where org.opengis interfaces are read-only.

  This is an important distinction, having immutable interfaces avoids requirement to copies when sharing a data structure between threads. The filter and style classes have a lot of workarounds with clone() and cast() methods to bridge this difference.

• GeoTools interfaces often have extra helper methods to make the data structure easier to use (and thus may not be minimal).

  Use of interface default methods for extra helper methods can provide ease-of-use comforts while avoiding requiring boiler-plate code in each implementation.

  public Object accept(StyleVisitor visitor, Object data) {
    return visitor.visit(this, data);
  }

Consistent handling

One side effect of pulling down org.opengis.style and org.opengis.filter interfaces is that they are documented as being immutable, while the GeoTools equivalent classes are mutable with set methods.

This proposal takes a strong stance that the mutable objects are acting as builders for the immutable data structure shared between threads. The builders should be freely editable, with their own visitors for changing in place.

If my thinking is correct we can use the copy on write approach, so that the immutable object under construction is available to be shared at any time. It may even be possible to automate a bit of this with a dynamic proxy.

BEFORE

The org.opengis interfaces follow java bean naming conventions with get methods, but are documented as being immutable:

package org.opengis.style;

public interface Fill {
  GraphicFill getGraphicFill();
  Expression getColor();
  Expression getOpacity();
  Object accept(StyleVisitor visitor, Object extraData);
}

The org.geotools.styling interfaces follow java bean naming conventions with get and set methods, and are documented as being mutable:

package org.geotools.styling;

public interface Fill extends org.opengis.style.Fill {
  static final Fill DEFAULT;
  static final Fill NULL;

  Graphic getGraphicFill();
  void setGraphicFill(org.opengis.style.Graphic graphicFill);

  void accept(org.geotools.styling.StyleVisitor visitor);

  abstract class ConstantFill implements Fill;
}

Implementation set methods use cast convention to convert to mutable implementations (creating copy if required). Type narrowing on get methods to document that mutable implementations are being returned. These classes occasionally have ConstantInterface where the set methods throw an UnsupportedOperationException.

package org.geotools.styling;

public class FillImpl implements Fill, Cloneable {
  protected FillImpl();
  FillImpl(FilterFactory factory);
  setFilterFactory(FilterFactory factory);

  public Expression getColor();
  public void setColor(Expression rgb);
  public void setColor(String rgb);

  public Expression getOpacity();
  public void setOpacity(Expression opacity);
  public void setOpacity(String opacity);

  public org.geotools.styling.Graphic getGraphicFill();
  public void setGraphicFill(org.opengis.style.Graphic graphicFill);

  public Object accept(StyleVisitor visitor, Object data);
  public void accept(org.geotools.styling.StyleVisitor visitor);

  public int hashCode();
  public boolean equals(Object oth);

  public Object clone();
  static FillImpl cast(org.opengis.style.Fill fill)
}

AFTER copy on write

Following java bean naming conventions, with only get methods:

package org.geotools.styling;

public interface Fill {
  Expression getColor();
  Expression getOpacity();
  GraphicFill getGraphicFill();
  default Object accept(StyleVisitor visitor, Object extraData) {
    return visitor.visit(this)
  }
}

The default Fill data object implementation:

package org.geotools.styling;

public class FillImpl implements Fill {
  private Expression color;
  private Expression opacity;
  private GraphicFill fill;
  DefaultFill(Expression color, Expression opacity, GraphicFill fill){ ... }
  Expression getColor(){ return color; }
  Expression getOpacity(){ return opacity; }
  GraphicFill getGraphicFill(){ return fill; }
  int hashcode(){..}
  boolean equals(Object other){..}
}

Following java bean naming conventions get/set for a mutable builder, with lazy copy-on-write creation:

package org.geotools.styling;

public class FillBuilder {

  Fill obj;
  GraphicFillBuilder graphicFill;

  FillImpl(){...}
  FillImpl(FillBuilder copy){...}
  FillImpl(org.geotools.style.Fill copy){...}
  StyleFactory getStyleFactory(){...}
  void setStyleFactory(StyleFactory factory){...}
  void init(Fill fill){...}
  
  Expression getColor(){
    return obj.getColor();
  }
  void setColor(Expression color){
     if (getColor() != color){
       this.obj = getStyleFactory().fill( color, getOpacity(), getGraphicFill() );
     }
  }
  Expression getOpacity(){
    return obj.getOpacity();
  }
  void setOpacity(Expression opacity){
    if (getOpacity() != opacity) {
      this.obj = getStyleFactory().fill( getColor(), opacity, getGraphicFill() );
    }
  }
  GraphicFillBuilder getGraphicFill(){
     return this.graphicFill != null ? this.graphicFill.build() : this.object.getGraphicFill();
  }
  void setGraphicFill(GraphicFill graphicFill){
     if (getGraphicFill() != graphicFill) {
       this.graphicFill = new GraphicFillBuilder(graphicFill);
       this.graphicFill.setStyleBuilder(getStyleBuilder);
     }
  }

  void accept(org.geotools.styling.StyleBuilderVisitor visitor) {
    visitor.visit(this);
  }

  public int hashCode(){..}
  public boolean equals(Object other){..}

  Fill build(){
      GraphicFill fill = getGraphicFill();
      if( obj.getGraphicFill() != fill ){
         this.obj = getStyleFactory().fill( getColor(), getOpacity(), fill );
      }
      return this.obj;
  }
}

ALTERNATIVE: fluent api with immutable naming conventions

Following immutable naming conventions, with default methods for any boiler-plate code:

package org.geotools.styling;

public interface Fill {
  GraphicFill graphicFill();
  Fill graphicFill(GraphicFill graphicFill);
  Expression color();
  Fill color(Expression color);
  Expression opacity();
  Fill opacity(Expression opacity);
  default Object accept(StyleVisitor visitor, Object extraData) {
    return visitor.visit(this)
  }
}

Implementation:

package org.geotools.styling;

public class FillImpl implements Fill {
   StyleFactory factory;
   Expression color;
   Expression opacity;   
   public FillImpl(StyleFactory factory){
      this.factory = factory;
   }
   public Expression color(){
    return color;
  }
  Fill color(Expression color){
    return factory.fill(color,this.opacity,this.graphicFill);
  }
  Expression opacity(){
    return this.opacity;
  }
  Fill opacity(Expression opacity){
    return factory.fill(this.color,opacity,this.graphicFill);
  }
 Graphic graphicFill(){
    return this.graphicFill;
  }
  Fill graphicFill(Graphic graphicFill){
    return factory.fill(this.color,this.opacity,graphicFill);
  }  
  default void accept(org.geotools.styling.StyleVisitor visitor) {
    visitor.visit(this);
  }
}

To help with migration a builder can be created following get/set naming conventions, with build() being called before use:

package org.geotools.styling;

public class FillBuilder extends Buildable<Fill> {
  StyleFactory factory;
  Expression color;
  Expression opacity;
  GraphicBuilder graphicFill;
  public FillBuilder(StyleFactory factory){
    this.factory = factory;
  }
  public FillBuilder init(Fill fill){
     this.color = fill.color();
     this.opacity = opacity();
     this.graphic graphicFill = new GraphicBuilder().init(fill.graphicFill);
  }
  public StyleFactory getStyleFactory(){
    return this.factory;
  }
  public FilterFactory getFilterFactory(){
    return this.factory. getFilterFactory();
  }
  public Expression color(){
    return getColor();
  }
  public Fill color(Expression color){
    setColor(color);
    return build();
  }
  public Expression getColor();
  public void setColor(Color color);
  public void setColor(String rgb) {
     setColor( getFilterFactory().literal(rgb) );
  }

  public Expression opacity(){
    return getColor();
  }
  public Fill opacity(Expression opacity){
    setOpacity(opacity);
    return build();
  }
  public Expression setOpacity();
  public void setOpacity(Expression opacity);
  public void setOpacity(float opacity) {
     setOpacity( getFilterFactory().literal(opacity) );
  }

  public Graphic graphicFill(){
    return getGraphic();
  }
  public Fill graphicFill(Graphic graphicFill){
    setGraphicFill(graphicFill);
    return build();
  }
  Graphic getGraphicFill(){
    return this.graphicFill.build();
  }
  setGraphicFill(Graphic graphicFill){
    this.graphicFill.init(graphicFill);
  }
  public void accept(StyleBuilderVisitor visitor) {
    visitor.visit(this);
  }
  public Fill build() {
    return factory.fill(color,opacity,graphicFill.build());
  }
}