effective-java-3e-source-code icon indicating copy to clipboard operation
effective-java-3e-source-code copied to clipboard

Abstract builders should not declare an abstract `build()` method

Open koritakoa opened this issue 3 years ago • 0 comments

...and certainly not a package-private one.

In Item 2, the abstract Pizza class declares an abstract Builder class. This abstract builder declares an abstract build method: abstract Pizza build();. I argue that this is unnecessary and incorrect.

The problem is that by declaring the abstract build method without checked exceptions, the abstract builder class is forbidding subclasses from throwing checked exceptions from the build method.

The builder pattern is being used in place of constructors here. Having a build method that prevents subclasses from throwing checked exceptions is like having a constructor that prevents subclasses from throwing checked exceptions in their constructors.

It is incorrect of the author of the abstract builder class to assume that subclasses would not wish to throw checked exceptions from their build methods or from the constructors these build method call.

I argue that declaring the abstract build method in the abstract builder is unnecessary because it is not the concern of the author of the abstract builder class. The author of the abstract builder class should be concerned with writing setters for values required by the constructor and retrieving these values in the constructor of the target class.

If a subclass of the builder does not have a build method that results in the builder being passed to the target type constructor, then it is a failure on the part of the subclass, not the abstract builder superclass.

I acknowledge that constructors should be used to construct objects, not do extensive work. As such, we should probably be mostly throwing RuntimeExceptions from constructors because of validation errors. However, sometimes constructors do throw checked exceptions (like FileOutputStream). We may also wish to do work which may throw a checked exception in the build method itself before calling the constructor of a subclass.

I also acknowledge that declaring the abstract build method helps to create subclasses of the builder with consistently named build methods. Note that because the abstract build method is declared with package-private access, this consistency is only probable (subclass authors will probably do the logical thing and override it with a public access modifier - but they may do something weird), and not really that useful. The method is not part of a consistent API shared by all instances of builders. All instances of builders probably have a build method, but other classes cannot call Pizza.Builder.build(), only NyPizza.Builder.build() etc. You cant do this:

Collection<Pizza.Builder<?>> l = something();
l.stream().map(Pizza.Builder::build)

With package-private access, the abstract build method is not useful enough to warrant forbidding subclasses from throwing checked exceptions.

If a consistent API across subclasses of Pizza.Builder is required then the build method should be declared public in the abstract builder, and the author of the abstract builder should consider allowing subclasses to throw checked exceptions.

I think authors of abstract builders should ask themselves if their task it to make their target class constructor easier to use or if their task is to write API for a factory of their target class.

koritakoa avatar Sep 17 '21 10:09 koritakoa