Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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;
    }
}
share|improve this question
1  
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. –  InstructedA Jul 15 at 4:57

1 Answer 1

up vote 5 down vote accepted

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.

share|improve this answer
    
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 at 7:21

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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