5

I am having trouble evaluating an implementation of the builder pattern I just came up with.

The context is an API library, so I am trying not to expose any implementations in order to have a stable interface while being able to change implementations later.

My thinking is that even though there is a reference to the implementation in the interface's file, the interface and the implementation are not actually coupled because

In effect, a static nested class is behaviorally a top-level class...

The benefit of this design is that clients can call

Person person = new Person.Builder().age(20).firstName("John") .lastName("Doe").build();

without the need for a dedicated Factory type class.

Am I missing something or is this a valid design decision?

Person.java

package net.mhi.rd;

public interface Person {

    int getAge();
    String getFirstName();
    String getLastName();

    public static class Builder {
        int age;
        String firstName;
        String lastName;

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

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

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

        public Person build() {
            return new PersonImpl(this);
        }
    }    
}

PersonImpl.java

package net.mhi.rd;

class PersonImpl implements Person {

    private final int age;
    private final String firstName;
    private final String lastName;

    PersonImpl(Person.Builder builder) {
        this.age = builder.age;
        this.firstName = builder.firstName;
        this.lastName = builder.lastName;
    }

    @Override
    public int getAge() {
        return age;
    }

    @Override
    public String getFirstName() {
        return firstName;
    }

    @Override
    public String getLastName() {
        return lastName;
    }
}
  • 2
    At the low-level details, you are right static inner class is almost the same as a separate class especially in the perspective of what you are trying to do. However, just keep it outside is a good practice because it is part of the implementation of the interface. – InformedA Jul 15 '14 at 4:57
10

The main problem is that you've created a circular dependency between Person and PersonImpl... Person shouldn't have any references to implementation classes, but the "new PersonImpl()" call in the builder creates a hard, compile-time dependency on the concrete class.

  • I think the fix is to either spin PersonBuilder off into it's own class, or else move PersonBuilder into PersonImpl. It's a little weird declaring a class inside an interface anyway -- I wouldn't say I've never done that, but it seems preferable to keep interfaces simple.

  • Passing the Builder as a constructor argument in PersonImpl couples the implementation class really tightly to the builder. You'd be better off keeping a cleaner separation between the builder and the buildee. The builder (obviously) needs to know about PersonImpl, but it would be best if PersonImpl didn't reference the builder.

  • Do you anticipate having more than one Person implementation? It's looks like a data class, and I'm not sure you need to extract an interface in the first place.

  • And... as a general pattern, if you're going to create a dozen or so classes like this, I think this pattern would become pretty cumbersome -- there's an interface, implementation, and builder for each class. That seems like a lot of extra code for something that may not need distinct interfaces.

Here's the variation to Builder that Josh Block came up with. It's similar in that it uses an inner class to access & set internal values, so that the class becomes effective immutable. I think the main difference with your code is that there's not a separate interface.

http://rwhansen.blogspot.com/2007/07/theres-builder-pattern-that-joshua.html

TL;DR:

  • the dependencies should go Builder --> Impl --> Interface

  • omitting the interface and nesting the Builder inside the Impl is a nice way to provide one-time access to the internal impl details, while hiding a circular dependency between builder and impl.

  • You are right, the circular dependency is problematic. One could resolve this by mimicking the Builder's build() method from the example you provided and setting the PersonImpl's fields to protected. But then one would need to keep the package very small to keep the class effectively immutable. – mhi Jul 15 '14 at 7:21
3

I very much agree with the opening paragraph and point #3 made by Rob. I am going to elaborate on those because they are in my opinion the most serious design flaws.

You would only really need to create an interface for Person if you are planning to create more than one kind of Person implementation. Otherwise, there really isn't any reason to have it. Someone might argue that having the Person interface allows you to change the PersonImpl class without affecting the rest of the program. More specifically, the interface prevents any outside parts of the program from communicating directly with PersonImpl, which we know is good for loose coupling. However, if there is only ever going to be one Implementation of the Person interface, then that would mean there is no purpose in having it and it would just be extra code. Extra code increases complexity and increased complexity makes the code harder to read, understand and maintain.

Under the assumption that you do plan on having more than 1 kind of Person then having an explicit reference to the implementing class in your interface is a mistake because it makes it difficult to extend your interface to new implementations. Always remember why you're doing what you're doing. In the case of designing interfaces, we are always trying to achieve greater flexibility and maintainability by ensuring that we rarely have to modify existing code, because modifying existing code usually results in introducing bugs. In your interface, you have PersonImpl stated in the build() method. This is a problem because everytime you want to make a new implementation of the Person interface you would need to modify the code in the Interface's class. In this case, it might not be a problem because there isn't a lot of code, but as the code grows it's very easy for something like this to get lost in the code.

enter image description here

As a personal note, I recommend complementing code with UML or a similar graphic representation when communicating your design ideas. They are designed specifically for that purpose and in almost all cases they have helped me identify design flaws in my code which I would not have identified otherwise.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.