Java – Improvements to Joshua Bloch’s Builder Design Pattern

design-patternsjava

Back in 2007, I read an article about Joshua Blochs take on the "builder pattern" and how it could be modified to improve the overuse of constructors and setters, especially when an object has a large number of properties, most of which are optional. A brief summary of this design pattern is articled here.

I liked the idea, and have been using it since. The problem with it, while it is very clean and nice to use from the client perspective, implementing it can be a pain in the bum! There are so many different places in the object where a single property is reference, and thus creating the object, and adding a new property takes a lot of time.

So…I had an idea. First, an example object in Joshua Bloch's style:

Josh Bloch Style:

public class OptionsJoshBlochStyle {

    private final String option1;
    private final int option2;
    // ...other options here  <<<<

    public String getOption1() {
        return option1;
    }

    public int getOption2() {
        return option2;
    }

    public static class Builder {

        private String option1;
        private int option2;
        // other options here <<<<<

        public Builder option1(String option1) {
            this.option1 = option1;
            return this;
        }

        public Builder option2(int option2) {
            this.option2 = option2;
            return this;
        }

        public OptionsJoshBlochStyle build() {
            return new OptionsJoshBlochStyle(this);
        }
    }

    private OptionsJoshBlochStyle(Builder builder) {
        this.option1 = builder.option1;
        this.option2 = builder.option2;
        // other options here <<<<<<
    }

    public static void main(String[] args) {
        OptionsJoshBlochStyle optionsVariation1 = new OptionsJoshBlochStyle.Builder().option1("firefox").option2(1).build();
        OptionsJoshBlochStyle optionsVariation2 = new OptionsJoshBlochStyle.Builder().option1("chrome").option2(2).build();
    }
}

Now my "improved" version:

public class Options {

    // note that these are not final
    private String option1;
    private int option2;
    // ...other options here

    public String getOption1() {
        return option1;
    }

    public int getOption2() {
        return option2;
    }

    public static class Builder {

        private final Options options = new Options();

        public Builder option1(String option1) {
            this.options.option1 = option1;
            return this;
        }

        public Builder option2(int option2) {
            this.options.option2 = option2;
            return this;
        }

        public Options build() {
            return options;
        }
    }

    private Options() {
    }

    public static void main(String[] args) {
        Options optionsVariation1 = new Options.Builder().option1("firefox").option2(1).build();
        Options optionsVariation2 = new Options.Builder().option1("chrome").option2(2).build();

    }
}

As you can see in my "improved version", there are 2 less places in which we need to add code about any addition properties (or options, in this case)! The only negative that I can see is that the instance variables of the outer class are not able to be final. But, the class is still immutable without this.

Is there really any downside to this improvement in maintainability? There has to be a reason which he repeated the properties within the nested class that I'm not seeing?

Best Answer

Your variation is quite nice. But it lets users do this:

Options.Builder builder = new Options.Builder().option1("firefox").option2(1);
Options optionsVariation1 = builder.build();
assert optionsVariation1.getOption1().equals("firefox");
builder.option1("chrome");
assert optionsVariation1.getOption1().equals("firefox"); // FAILURE!

Which rather defeats the object.

You could change the build method to do this:

public Options build() {
    Options options = this.options;
    this.options = null;
    return options;
}

Which would prevent this - any call to a setter method on the builder after the build call will fail with a NullPointerException. If you wanted to be flash, you could even test for null and throw an IllegalStateException or something instead. And you could move that up to a generic base class where it could be used across all builders.

Related Solutions

Java – Object-Oriented Class Design

Example 2 is quite bad for testing... and I don't mean that you can't test the internals. You also can't replace your XmlReader object by a mock object as you have no object at all.

Example 1 is needlessly hard to use. What about

XmlReader reader = new XmlReader(url);
Document result = reader.getDocument();

which is not any harder to use than your static method.

Things like opening the URL, reading XML, converting bytes to strings, parsing, closing sockets, and whatever, are uninteresting. Creating an object and using it is important.

So IMHO the proper OO Design is to make just the two things public (unless you really need the intermediate steps for some reason). Static is evil.

Java Design Patterns – Why Types Are Coupled with Their Builders

I disagree with your assertion that it is a red flag. I also disagree with your representation of the counterpoint, that there is One Right Way of representing a Builder pattern.

To me there's one question: Is the Builder a necessary part of the type's API? Here, is the PersonBuilder a necessary part of the Person API?

It is entirely reasonable that a validity-checked, immutable, and/or tightly-encapsulated Person class will necessarily be created through a Builder it provides (regardless of whether that builder is nested or adjacent). In doing so, the Person can keep all of its fields private or package-private and final, and also leave the class open for modification if it is a work in progress. (It may end up closed for modification and open for extension, but that's not important right now, and is especially controversial for immutable data objects.)

If it is the case that a Person is necessarily created through a supplied PersonBuilder as part of a single package-level API design, then a circular coupling is just fine and a red flag isn't warranted here. Notably, you stated it as an incontrovertible fact and not an API design opinion or choice, so that may have contributed to a bad response. I wouldn't have downvoted you, but I don't blame someone else for doing so, and I'll leave the rest of the discussion of "what warrants a downvote" to the Code Review help center and meta. Downvotes happen; it's no "slap".

Of course, if you want to leave a lot of ways open to build a Person object, then the PersonBuilder could become a consumer or utility of the Person class, on which the Person should not directly depend. This choice seems more flexible—who wouldn't want more object creation options?—but in order to hold to guarantees (like immutability or serializability) suddenly the Person has to expose a different kind of public creation technique, such as a very long constructor. If the Builder is meant to represent or replace a constructor with a lot of optional fields or a constructor open to modification, then the class's long constructor may be an implementation detail better left hidden. (Also bear in mind that an official and necessary Builder doesn't preclude you from writing your own construction utility, just that your construction utility may consume the Builder as an API as in my original question above.)

p.s.: I note that the code review sample you listed has an immutable Person, but the counterexample from Wikipedia lists a mutable Car with getters and setters. It may be easier to see the necessary machinery omitted from the Car if you keep the language and invariants consistent between them.

Best Answer

Related Solutions

Java – Object-Oriented Class Design

Java Design Patterns – Why Types Are Coupled with Their Builders

Related Topic