Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abstract builders should not declare an abstract build() method #24

Open
koritakoa opened this issue Sep 17, 2021 · 0 comments
Open

Abstract builders should not declare an abstract build() method #24

koritakoa opened this issue Sep 17, 2021 · 0 comments

Comments

@koritakoa
Copy link

...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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant