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.
–
InstructedAJul 15 '14 at 4:57

1 Answer
1

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.

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.
–
mhiJul 15 '14 at 7:21