Why are class members duplicated in the builder pattern?

Question

The builder pattern (in Java) is usually illustrated by an example like this:

public class MyClass {

    private String member1;

    private String member2;

    // Getters & setters

    public class MyClassBuilder {

        private String nestedMember1;

        private String nestedMember2;

        public MyClassBuilder withMember1(String member1) {
            this.nestedMember1 = member1;
            return this;
        }

        public MyClassBuilder withMember2(String member2) {
            this.nestedMember1 = member1;
            return this;
        }

        public MyClass build() {
            MyClass myClass = new MyClass();

            myClass.member1 = nestedMember1;
            myClass.member2 = nestedMember2;

            return myClass;
        }

    }

}

So we get two very similar classes and since this pattern is mostly applied when we have a big number of members, we get a big amount of similarities. This violates the DRY principle in my opinion.

What is wrong with the following code?

public class MyClass {

    private String member1;

    private String member2;

    // Getters & setters

    public class MyClassBuilder {

        private MyClass myClass = new MyClass();

        public MyClassBuilder withMember1(String member1) {
            myClass.member1 = member1;
            return this;
        }

        public MyClassBuilder withMember2(String member2) {
            myClass.member2 = member1;
            return this;
        }

        public MyClass build() {
            return myClass;
        }

    }

}

Answer 1

So we get two very similar classes [...]. This violates the DRY principle in my opinion.

That's debatable. After all, the fields in builder class may sometimes be totally different than the ones in the product class. And even when they are similar, often only a subset of product's fields needs to be represented in the builder.

Given the definition of DRY ("Every piece of knowledge must have a single, unambiguous, authoritative representation within a system."), an argument can be made that the common implementation doesn't violate DRY, since product's fields and builder's fields, even if they coincide by name, actually represent two different pieces of knowledge: the former represent information as to the internal state of an existing object, whereas the latter represent the data, needed to create another instance of the product.

Anyway, let's get to the more practical part.

What is wrong with the following code?

Generally speaking, it would be reasonable for a programmer, using your MyClassBuilder, to expect that each subsequent call to build() on the same builder instance would create a new instance of MyClass or, at least, throw an exception, if the reuse of builder instances is not supported by your implementation.

Instead, your implementation returns one and the same pre-constructed MyClass instance on each call to build(). In many cases, this behavior might present a few unpleasant surprises to the users of MyClassBuilder. Consider the following examples:

// Example 1
MyClass.MyClassBuilder builder = new MyClass.MyClassBuilder()
    .withMember1("foo")
    .withMember2("bar");

MyClass product = builder.build();

builder.withMember2("baz");

// The following line prints out "baz", instead of "bar"...
// But wait, I haven't even touched product!
System.out.println(product.getMember2());


// Example 2
MyClass.MyClassBuilder builder = new MyClass.MyClassBuilder()
    .withMember1("hello")
    .withMember2("world");

MyClass product1 = builder.build();
MyClass product2 = builder.build();

product1.setMember1("bye");

// The following line prints out "bye", instead of "hello"...
// Wait, what??!
System.out.println(product2.getMember1());

Then the hypothetical programmer would realize that all the products are actually one and the same object (to which the builder has been kindly returning references each time they called build()). And then they would be quite exasperated indeed.

Your suggested implementation could actually meet the expectations, outlined above, if MyClass supported cloning or had a copy constructor. In this case, build() would be able to copy MyClassBuilder's internal MyClass instance and return a new copy on every call. For the sake of simplicity, let's add a private copy constructor:

public class MyClass {

    private String member1;

    private String member2;

    public String getMember1() {
        return member1;
    }

    public String getMember2() {
        return member2;
    }

    // Getters & setters

    private MyClass() {

    }

    /**
     * Copy constructor to be used by builder.
     * @param other Original MyClass instance.
     */
    private MyClass(MyClass other) {
        // Strings are immutable, so we can simply copy references.
        // In general, you should consider whether a deep copy needs to be performed for a field.
        this.member1 = other.member1;
        this.member2 = other.member2;
    }

    public static class MyClassBuilder {

        private MyClass myClass = new MyClass();

        public MyClassBuilder withMember1(String member1) {
            myClass.member1 = member1;
            return this;
        }

        public MyClassBuilder withMember2(String member2) {
            myClass.member2 = member1;
            return this;
        }

        public MyClass build() {
            return new MyClass(myClass);    // The client will now get a copy
        }
    }
}

At this point one might notice that our Builder implementation starts to look more and more like a glorified Prototype. In fact, if we were to replace our private copy constructor with a public clone() method, MyClass would literally become an example of Prototype and at that point, in most cases, there would no longer be any need for MyClassBuilder at all. Still, such an implementation of Builder works just fine.

---

There is another, arguably, more minor, problem with your approach: it prevents the fields in MyClass, whose values are set during building process, from being declared final. This is not such a big deal for the client code, since you can simply choose not to provide public setters for said fields. However, it's still nice to have fields, intended to be final, explicitly declared as such in MyClass, since it prevents programmers, working on the code of MyClass, from mistakenly changing their value, thus introducing hard to find bugs.

Now, the "common" implementation, as you present it, also suffers from this issue, but there it could be addressed by introducing another constructor to MyClass and rewriting builder's build() method as follows:

    public MyClass build() {
        return new MyClass(nestedMember1, nestedMember2);
    }

---

As for implementing Builder in Java in general, I should point out that the inner builder class needs to be declared static, so that the outside code, or a convenience static method (getBuilder()) in MyClass, could construct it without needing an existing instance of MyClass. (This is true for both implementations in your question, so I reckon the lack of static was a typo on your part.) Another minor point: it's a good idea to give MyClass a private constructor, if you prefer clients to use builder class and don't want them to construct MyClass instances directly.