More on Builders, or, favor immutability

A while back I posted a basic Builder example on Stack Overflow. User Marcel Stör complained:

Problem with this approach is that you need to define all the members twice, once in the builder and once in the actual class. [This answer] shows how this can be avoided. With that other approach, however, you cannot have final members although the object itself is still immutable.

Marcel Stör

Well, I looked at that answer, from user Yishai, and I didn’t like it.

I don’t know, I’m not a fan of that approach — the builder’s not reusable, it’s not thread-safe, and you can’t design the build product class to guarantee it’s fully initialized at construction. If I was really worried about DRY, I’d probably use an immutable (copy-on-write) builder and make the last builder instance double as the build product’s immutable data.

David Moles

After I wrote that, I thought I should put some code where my mouth was. So: here’s the original version (more or less), with the fields declared in both the Builder and its product, the Widget:

public class Widget implements IWidget
{
    public static class Builder implements IWidgetBuilder<Widget>
    {
        private String name;
        private String model;
        private String upc;
        private double price;
        private Manufacturer manufacturer;

        public Builder ( String name, double price )
        {
            this.name = name;
            this.price = price;
        }

        @Override
        public Widget build ()
        {
            Widget product = new Widget( this );
            validate( product );
            return product;
        }

        @Override
        public Builder manufacturer ( Manufacturer value )
        {
            this.manufacturer = value;
            return this;
        }

        @Override
        public Builder upc ( String value )
        {
            this.upc = value;
            return this;
        }

        @Override
        public Builder model ( String value )
        {
            this.model = value;
            return this;
        }

        private static void validate ( Widget product )
        {
            assert product.getName() != null : "Product must have a name";
            assert product.getPrice() >= 0 : "Product price cannot be negative";
            Manufacturer mfgr = product.getManufacturer();
            if ( mfgr != null )
            {
                String upc = product.getUpc();
                assert upc != null;
                assert upc.startsWith( mfgr.getCode() ) : "Product UPC does not match manufacturer";
            }
        }
    }

    private final String name;
    private final String model;
    private final String upc;
    private final double price;
    private final Manufacturer manufacturer;

    /**
     * Creates an immutable widget instance.
     */
    private Widget ( Builder b )
    {
        this.name = b.name;
        this.price = b.price;
        this.model = b.model;
        this.upc = b.upc;
        this.manufacturer = b.manufacturer;
    }

    public Manufacturer getManufacturer ()
    {
        return manufacturer;
    }

    public String getModel ()
    {
        return model;
    }

    public String getName ()
    {
        return name;
    }

    public double getPrice ()
    {
        return price;
    }

    public String getUpc ()
    {
        return upc;
    }
}

And here’s the new version: the fields are only declared in the Builder, and they’re always final. The Builder itself is immutable, using a copy-on-write pattern, and the last Builder instance becomes the state of the Widget it produces.

public class Widget2 implements IWidget
{
    public static class Builder implements IWidgetBuilder<Widget2>
    {
        private final String name;
        private final String model;
        private final String upc;
        private final double price;
        private final Manufacturer manufacturer;

        public Builder ( String name, double price )
        {
            this( name, null, null, price, null );
        }

        private Builder ( String name, String model, String upc, double price, Manufacturer manufacturer )
        {
            this.manufacturer = manufacturer;
            this.model = model;
            this.name = name;
            this.price = price;
            this.upc = upc;
        }

        public Widget2 build ()
        {
            Widget2 product = new Widget2( this );
            validate( product );
            return product;
        }

        public Builder manufacturer ( Manufacturer value )
        {
            return new Builder( this.name, this.model, this.upc, this.price, value );
        }

        public Builder upc ( String value )
        {
            return new Builder( this.name, this.model, value, this.price, this.manufacturer );
        }

        public Builder model ( String value )
        {
            return new Builder( this.name, value, this.upc, this.price, this.manufacturer );
        }

        private static void validate ( Widget2 product )
        {
            assert product.getName() != null : "Product must have a name";
            assert product.getPrice() >= 0 : "Product price cannot be negative";
            Manufacturer mfgr = product.getManufacturer();
            if ( mfgr != null )
            {
                String upc = product.getUpc();
                assert upc != null;
                assert upc.startsWith( mfgr.getCode() ) : "Product UPC does not match manufacturer";
            }
        }

    }

    private final Builder b;

    /**
     * Creates an immutable widget instance.
     */
    private Widget2 ( Builder b )
    {
        this.b = b;
    }

    @Override
    public Manufacturer getManufacturer ()
    {
        return b.manufacturer;
    }

    @Override
    public String getModel ()
    {
        return b.model;
    }

    @Override
    public String getName ()
    {
        return b.name;
    }

    @Override
    public double getPrice ()
    {
        return b.price;
    }

    @Override
    public String getUpc ()
    {
        return b.upc;
    }
}

Voilà, DRY. Sadly, it only ends up one line shorter (thanks to the extra Builder constructor), but at least it doesn’t define anything twice!

Subclassing with Bloch’s Builder pattern, revised

So last year I posted about using Joshua Bloch’s Builder pattern to create objects from a hierarchy of subclasses.

I realized later that while I did have something like this completely working at one point, I had it working in C#, which has deceptively different generic semantics. (It also has named parameters, which makes the Builder pattern a fair bit less necessary.) As Zdenek Henek demonstrated (several weeks ago, sorry Zdenek!), in Java, the original version I posted doesn’t allow you to call the arguments in any order.

This one does:

class Shape
{
    private final double opacity;

    public double getOpacity ()
    {
        return opacity;
    }

    public static abstract class ShapeBuilder<S extends Shape, B extends ShapeBuilder<S, B>>
    {

        private double opacity;

        @SuppressWarnings( "unchecked" )
        public B opacity ( double opacity )
        {
            this.opacity = opacity;
            return (B) this;
        }

        public abstract S build ();
    }

    private static class DefaultShapeBuilder extends ShapeBuilder<Shape, DefaultShapeBuilder>
    {
        @Override
        public Shape build ()
        {
            return new Shape( this );
        }
    }

    public static ShapeBuilder<?, ?> builder ()
    {
        return new DefaultShapeBuilder();
    }

    protected Shape ( ShapeBuilder<?, ?> builder )
    {
        this.opacity = builder.opacity;
    }
}

class Rectangle extends Shape
{

    private final double height;
    private final double width;

    public double getHeight ()
    {
        return height;
    }

    public double getWidth ()
    {
        return width;
    }

    public static abstract class RectangleBuilder<S extends Rectangle, B extends RectangleBuilder<S, B>> extends ShapeBuilder<S, B>
    {
        private double height;
        private double width;

        @SuppressWarnings( "unchecked" )
        public B height ( double height )
        {
            this.height = height;
            return (B) this;
        }

        @SuppressWarnings( "unchecked" )
        public B width ( double width )
        {
            this.width = width;
            return (B) this;
        }
    }

    public static RectangleBuilder<?, ?> builder ()
    {
        return new DefaultRectangleBuilder();
    }

    protected Rectangle ( RectangleBuilder<?, ?> builder )
    {
        super( builder );
        this.height = builder.height;
        this.width = builder.width;
    }

    private static class DefaultRectangleBuilder extends RectangleBuilder<Rectangle, DefaultRectangleBuilder>
    {
        @Override
        public Rectangle build ()
        {
            return new Rectangle( this );
        }
    }
}

class RotatedRectangle extends Rectangle
{
    private final double theta;

    public double getTheta ()
    {
        return theta;
    }

    public static abstract class RotatedRectangleBuilder<S extends RotatedRectangle, B extends RotatedRectangleBuilder<S, B>> extends Rectangle.RectangleBuilder<S, B>
    {
        private double theta;

        @SuppressWarnings( "Unchecked" )
        public B theta ( double theta )
        {
            this.theta = theta;
            return (B) this;
        }
    }

    public static RotatedRectangleBuilder<?, ?> builder ()
    {
        return new DefaultRotatedRectangleBuilder();
    }

    protected RotatedRectangle ( RotatedRectangleBuilder<?, ?> builder )
    {
        super( builder );
        this.theta = builder.theta;
    }

    private static class DefaultRotatedRectangleBuilder extends RotatedRectangleBuilder<RotatedRectangle, DefaultRotatedRectangleBuilder>
    {
        @Override
        public RotatedRectangle build ()
        {
            return new RotatedRectangle( this );
        }
    }
}

class BuilderTest
{
    public static void main ( String[] args )
    {
        RotatedRectangle rotatedRectangle = RotatedRectangle.builder()
                .theta( Math.PI / 2 )
                .width( 640 )
                .height( 400 )
                .height( 400 )
                .opacity( 0.5d )
                .width( 111 )
                .opacity( 0.5d )
                .width( 222 )
                .height( 400 )
                .width( 640 )
                .width( 640 )
                .build();
        System.out.println( rotatedRectangle.getTheta() );
        System.out.println( rotatedRectangle.getWidth() );
        System.out.println( rotatedRectangle.getHeight() );
        System.out.println( rotatedRectangle.getOpacity() );
    }
}

Note though it requires some unchecked casts, and depends on the convention that each Builder’s generics are self-referential, which could present some risk if it’s extended further. Test early, test often.

Subclassing with Bloch’s Builder pattern

Update: There’s a newer version of the code in this post; the newer version allows you to call the arguments in any order.


A co-worker of mine recently ran across this post by Eamonn McManus, “Using the builder pattern with subclasses,” and after due consideration, settled on what McManus calls “a shorter, smellier variant.”

Shorter because:

  1. it has only one Builder class per class

Smellier because:

  1. it uses raw types
  2. the builder’s self() method requires an unchecked cast

Frankly, I don’t find the shortness here a compelling argument for the smell, but I also think there’s more shortness to be found in McManus’ design while remaining fragrant. Thus:

class Shape
{
    private final double opacity;

    public double getOpacity()
    {
        return opacity;
    }

    public static abstract class Builder<T extends Shape> {

        private double opacity;

        public Builder<T> opacity(double opacity) {
            this.opacity = opacity;
            return this;
        }

        public abstract T build();
    }

    public static Builder<?> builder() {
        return new Builder<Shape>()
        {
            @Override
            public Shape build()
            {
                return new Shape(this);
            }
        };
    }

    protected Shape(Builder<?> builder) {
        this.opacity = builder.opacity;
    }
}

class Rectangle extends Shape {

    private final double height;
    private final double width;

    public double getHeight()
    {
        return height;
    }

    public double getWidth()
    {
        return width;
    }

    public static abstract class Builder<T extends Rectangle> extends Shape.Builder<T> {
        private double height;
        private double width;

        public Builder<T> height(double height) {
            this.height = height;
            return this;
        }

        public Builder<T> width(double width) {
            this.width = width;
            return this;
        }
    }

    public static Builder<?> builder() {
        return new Builder<Rectangle>()
        {
            @Override
            public Rectangle build()
            {
                return new Rectangle(this);
            }
        };
    }

    protected Rectangle(Builder<?> builder) {
        super(builder);
        this.height = builder.height;
        this.width = builder.width;
    }
}

class RotatedRectangle extends Rectangle {
    private final double theta;

    public double getTheta()
    {
        return theta;
    }

    public static abstract class Builder<T extends RotatedRectangle> extends Rectangle.Builder<T> {
        private double theta;

        public Builder<T> theta(double theta) {
            this.theta = theta;
            return this;
        }
    }

    public static Builder<?> builder() {
        return new Builder<RotatedRectangle>()
        {
            @Override
            public RotatedRectangle build()
            {
                return new RotatedRectangle(this);
            }
        };
    }

    protected RotatedRectangle(Builder<?> builder) {
        super(builder);
        this.theta = builder.theta;
    }
}

class BuilderTest {
    public static void main(String[] args) {
        RotatedRectangle rotatedRectangle = RotatedRectangle.builder()
                .theta(Math.PI / 2)
                .width(640)
                .height(400)
                .opacity(0.5d)
                .build();
        System.out.println(rotatedRectangle.getTheta());
        System.out.println(rotatedRectangle.getWidth());
        System.out.println(rotatedRectangle.getHeight());
        System.out.println(rotatedRectangle.getOpacity());
    }
}

Now, some notes:

  1. Where McManus’ generics are of the self-referential, Builder<T extends Builder<T>> variety (cf. Enum), in mine a Builder<T> builds Ts. Personally I think this will give fewer developers headaches.
  2. My Builders don’t require a self() method; they simply return this.
  3. Like McManus’ preferred design, this one requires two Builders per class, one abstract and one concrete. However, the concrete one is an anonymous inner class, so it’s less obtrusive, and there’s only one “boilerplate” method per class. A little longer than the “smelly” version in that respect, but in my opinion livable.

The 21st century: ur doin it wrong

I know Oracle must be drilling way below the bottom of the barrel these days trying to find a way to monetize Larry Ellison’s vanity purchase get some return out of the Sun buyout, but hiding the JAX-RS specification under a bushel and charging US$50k to implement it commercially mostly strikes me as a great way to make sure no significant amount of RESTful web service development ever gets done in Java.

(Comments closed due to spam.)

Interesting Mockito / reflection feature & workaround

So, you have two interfaces:

interface SettingsProvider {
  Settings getSettings(String id);
}

and

interface InternalSettingsProvider extends SettingsProvider {
  @Override
  InternalSettings getSettings(String id);
}

You mock one of the second, and you stub the method:

InternalSettingsProvider mockProvider =
    mock(InternalSettingsProvider.class);
when(mockProvider.getSettings(“anId”)).thenReturn(someSettings);

You run some code that only knows about SettingsProvider, not InternalSettingsProvider. This code calls getSettings(“anId”). You’re expecting it to then make use of someSettings so…

…you get a NullPointerException.

It turns out that, with reflection, it actually makes a difference whether you call getSettings() on SettingsProvider or on InternalSettingsProvider. Each interface results in a separate Method object.

When the SettingsProvider#getSettings() call comes in, the matching code looks at the Methods it has registered for stubbing, and it finds one for InternalSettingsProvider#getSettings(), but none for SettingsProvider#getSettings(). So it returns the default value, which for an object is null.

Luckily, the first workaround you might think of does actually do the trick.

when(((SettingsProvider) mockProvider)
    .getSettings(“anId”)).thenReturn(someSettings);

I ran into this with Mockito, but I imagine it’ll come up in some other reflection-related contexts too.

No, I do not want fries with that

You would think that a development platform — a free, open-source development platform — that prides itself on its flexibility and modularity would give you a way to install only the pieces you want, or at least uninstall the pieces you don’t want. And up till recently, you would have been right. But now you’re not.

(Seriously, if I can’t get this crap uninstalled, I might actually break down and pay for that IDEA 8 upgrade I’d been planning to skip. If it would take bread out of the mouths of the children of the people who thought this distribution system was a good idea, I’d do it in a heartbeat.)

Thank you…

…to whoever at Sun decided it would be a good idea for java.awt.geom.Point2D and java.awt.Point to override equals() and hashCode() even though they’re mutable. That’s four hours of my life I’m never going to get back.

Fun, or rather no fun, with generics (updated)

So, I understand that the following doesn’t work, but why doesn’t it work?

interface Adapter {}

class Adaptulator<I> {
    <e, A extends I & Adapter> void add(Class extl, Class<A> intl) {
    	addAdapterFactory(new AdapterFactory(extl, intl));
    }
}

The add() method gives me a compile error, “Cannot specify any additional bound Adapter when first bound is a type parameter” (in Eclipse), or “Type parameter cannot be followed by other bounds” (in IDEA), take your pick.

Clearly you’re just Not Allowed to use the type parameter I there, before the &, and that’s that. (And before you ask, it doesn’t work if you switch ’em, because there’s no guarantee that I isn’t a concrete class.) But why not? I’ve looked through Angelika Langer’s FAQ and can’t find an answer.

Generally when some generics limitation seems arbitrary, it’s because you’ve created a situation where the type system can’t actually enforce correctness. But I don’t see what case would break what I’m trying to do here. I’d say maybe it has something to do with method dispatch after type erasure, but there’s only one add() method, so it’s not like there’s any ambiguity…

So what’s the problem?


Update: I cross-posted this to stackoverflow.com, and Bruno De Fraine pointed out that the reason multiple bounds were introduced in the first place was to control the erasure. Since I is just going to erase to Object, it doesn’t get you anything there. Unfortunately, all I wanted was the compile-time type safety, so I’ll have to come up with something else…

Closures now please

I’m trying to fix the event handling in some table code someone else wrote a few months back (where “fix” here means “support an ugly legacy global event model without just calling fireTableDataChanged() for every little thing”). I pretty much know what I need it to do, but setting up the test expectations is seriously painful. Among other things, I’m having to crawl the table model to build up “expected” sets of rows and columns for each event, over and over again, with a slightly different crawl condition for each test.

Closures would make this clean, or at least easy. But we don’t have them, so instead it’s:

	private static interface ColMatcher {
		boolean matches(TableColumn col, int index);
	}

	private Set findColumns(ColMatcher matcher) {
		Set expectedCols = new HashSet();
		for (int col = 0, numCols = tableModel.getColumnCount(); col < numCols; col++) {
			TableColumn column = tableModel.getColumn(col);
			if (matcher.matches(column, col)) {
				expectedCols.add(col);
			}
		}
		return Collections.unmodifiableSet(expectedCols);
	}

and then, over and over again:

	public void testSomeLegacyEvent() {
		final Set expectedRows = ...;
		final Set expectedCols = findColumns(new ColMatcher() {
			@Override public boolean matches(TableColumn column, int index) {
				return /* some condition */;
			}
		});

		final NastyLegacyEvent e = ...;

		SwingUtilities.invokeAndWait(new Runnable() {
			@Override public void run() {
				nastyLegacyEventSystem.fireLegacyEvent(e);

				Set affectedColumns = tmListener.getAffectedColumns();
				assertEquals(expectedCols, affectedColumns);
				for (int col : expectedCols) {
					assertEquals(expectedRows, tmListener.getAffectedRows(col));
				}
			}
		});
	}

There’s really got to be a better way