dysfunctional Builder pattern?

M

marlow.andrew

I have a question about a convention I have seen in a project I am
working on. The convention is that when a class has a constructor with
a lengthy argument list, a second class called a 'builder', is used to
construct instances.

Note that this is not the same as the builder pattern in the GoF. The
GoF pattern has an abstract builder interface and a concrete builder
for a start. And there's a director. And the point is to employ a
common construction process to create different representations.

Having said that, let us use some of the GoF terminology. Let us refer
to the type of the object being built as the Product.

The so-called builder that I have seen is used to avoid calling a ctor
that has a long argument list. For the sake of the argument we must
assume that there are some valid cases where it is legitimate to have
a long ctor arg list. Please, let's not get into the "if your arg list
is very long then the design is already bad" argument.

The Builder has a private data member for every private data member
that the Product has. The Builder sets all these private members to
default values. There are setter for each private data member. There
is a build() method that invokes the ctor of Product, supplying all
the private data members of the Builder. Thus, for any members whose
setter has been invoked, the set value will be employed, but for any
values that have not been set, the builder-defined default value will
be used. The setter methods by convention begin with 'with'. They also
return a reference to the Builder so that setters can be chained
together. Here is an example of a Builder being used:

someWidget = SomeWidgetBuilder.withFirstAttribute(firstVal).
withSixthAttribute(sixthVal).
withTenthAttribute(tenthVal).build();

Here, someWidget has at least 10 attributes but only 3 of them have
been set explicitly. You can very clearly see what they are, but what
is not so clear is what all the attributes are that someWidget has.

The problem I have seen, and why I do not like this 'builder', is that
I have seen Product classes where you HAVE to define valid values for
certain private members and there is no suitable default value. The
Builder hides this and allows you to construct an object that is
invalid. For example, consider the someWidget object. It might be that
secondAttribute needs to be set. The code shown does not set it
explicitly so the default value employed by the Builder will be used.
This could make the system fail only when the someWidget object has
cause to use its secondAttribute member. At that point it is
discovered to be invalid and the code blows up. Eventually the
developer figures out that the error is in the call to the builder
where withSecondAttribute has not been called.

My preference is to use the ctor to construct an object, even if the
ctor list is long. One can always comment it. Thus I would say:

someWidget = new SomeWidget(startTime, endTime, partNumber, sharpness,
0, false, "widgetName");

I would comment the literal constants 0 and false, to make it clear
that there are the metric size and homeMade flag respectively.
someWidget can now never be invalid.

Regards,

Andrew Marlow
 
M

Mark Space

I would comment the literal constants 0 and false, to make it clear
that there are the metric size and homeMade flag respectively.
someWidget can now never be invalid.


What if you set homeMade true by mistake, will the code blow up if the
value is incorrect?
 
D

Daniel Pitts

I have a question about a convention I have seen in a project I am
working on. The convention is that when a class has a constructor with
a lengthy argument list, a second class called a 'builder', is used to
construct instances.

Note that this is not the same as the builder pattern in the GoF. The
GoF pattern has an abstract builder interface and a concrete builder
for a start. And there's a director. And the point is to employ a
common construction process to create different representations.

Having said that, let us use some of the GoF terminology. Let us refer
to the type of the object being built as the Product.

The so-called builder that I have seen is used to avoid calling a ctor
that has a long argument list. For the sake of the argument we must
assume that there are some valid cases where it is legitimate to have
a long ctor arg list. Please, let's not get into the "if your arg list
is very long then the design is already bad" argument.

The Builder has a private data member for every private data member
that the Product has. The Builder sets all these private members to
default values. There are setter for each private data member. There
is a build() method that invokes the ctor of Product, supplying all
the private data members of the Builder. Thus, for any members whose
setter has been invoked, the set value will be employed, but for any
values that have not been set, the builder-defined default value will
be used. The setter methods by convention begin with 'with'. They also
return a reference to the Builder so that setters can be chained
together. Here is an example of a Builder being used:

someWidget = SomeWidgetBuilder.withFirstAttribute(firstVal).
withSixthAttribute(sixthVal).
withTenthAttribute(tenthVal).build();

Here, someWidget has at least 10 attributes but only 3 of them have
been set explicitly. You can very clearly see what they are, but what
is not so clear is what all the attributes are that someWidget has.

The problem I have seen, and why I do not like this 'builder', is that
I have seen Product classes where you HAVE to define valid values for
certain private members and there is no suitable default value.
The common approach to this problem is that the Builder's constructor
takes all required parameters, so that you must specify them.
The
Builder hides this and allows you to construct an object that is
invalid. For example, consider the someWidget object. It might be that
secondAttribute needs to be set. The code shown does not set it
explicitly so the default value employed by the Builder will be used.
This could make the system fail only when the someWidget object has
cause to use its secondAttribute member.
That is a flaw in the SomeWidget constructor which should throw an
IllegalArgumentException stating what about its construction is invalid.
At that point it is
discovered to be invalid and the code blows up. Eventually the
developer figures out that the error is in the call to the builder
where withSecondAttribute has not been called.

My preference is to use the ctor to construct an object, even if the
ctor list is long. One can always comment it. Thus I would say:

someWidget = new SomeWidget(startTime, endTime, partNumber, sharpness,
0, false, "widgetName");

I would comment the literal constants 0 and false, to make it clear
that there are the metric size and homeMade flag respectively.
someWidget can now never be invalid.
I think that there is a happy medium. Make your Builder's constructor
take all the required values (throwing exceptions if they aren't valid).
Make your "SomeWidget" constructor throw exceptions as well, just in
case someone does bypass the builder.

Hope this helps,
Daniel.
 
D

Daniel Pitts

Steven said:
It seems that SomeWidgetBuilder has at least one static method
withFirstAttribute, and probably has others, so you can choose which
attribute to start with. Can't it be designed instead to have just one
static method (or constructor) which takes the "required" attributes,
and then allows the "optional" ones to be set in the normal builder way,
so you get a compile-time check that the required attributes have at
least been specified?

I guess there's no way if you do that, however, to permit flexibility in
the ordering of the required attributes (unless they all had distinct
types), or we wouldn't be bothering with builders in the first place.
One solution, which may require a code generator, is to have a different
Class for every combination of required parameters. There are a few
variations, depending on whether you need optional values or not.

For example:
public class Circle {
final Point center;
final Length radius;

public static class CenterAt {
private final Point center;
private CenterAt(Point center) { this.center = center; }
public Circle withRadius(Length radius) {
return new Circle(center, radius);
}
}

public static class WithRadius {
private final Length radius;
private WithRadius(Length radius) { this.radius = radius; }
public Circle centeredAt(Point center) {
return Circle.centeredAt(center).withRadius(radius);
}
}

public static CenterAt centeredAt(Point point) {
return new CenterAt(point);
}

public static WithRadius withRadius(Length length) {
return new WithRadius(length);
}
}

Now you can do interesting things like
Circle c = Circle.centeredAt(myPoint).withRadius(myLength);

Obviously, if you have three or more required fields, then you'll be
better off using a code generator. You'll end up with n! factory
classes, but an API that enforces required arguments at compile time.

Well, you'll still need to check for null or other invariants of course.
 
M

marlow.andrew

You could rewrite the SomeWidgetBuilder constructor to require those
undefaultable values as parameters.  That way you can be sure that the
SomeWidgetBuilder has the correct data for all the undefaultable
fields.

Your sample code would then look something like:

someWidget = new SomeWidgetBuilder(undefaultableAttributeVal).
                                 withFirstAttribute(firstVal).
                                 withSixthAttribute(sixthVal).
                                 withTenthAttribute(tenthVal).build();

Well, this sounds like the best solution, to me. Thanks!
I suggest that you read Bloch, "EffectiveJava", 2nd edition which
covers the Builder pattern at Item 2.

I will have a look. I do have a copy of EffectiveJava but I don't
recall that one.

-Andrew Marlow
 
T

Tom Anderson

[...] Here is an example of a Builder being used:

someWidget = SomeWidgetBuilder.withFirstAttribute(firstVal).
withSixthAttribute(sixthVal).
withTenthAttribute(tenthVal).build();
The problem I have seen, and why I do not like this 'builder', is that
I have seen Product classes where you HAVE to define valid values for
certain private members and there is no suitable default value. The
Builder hides this and allows you to construct an object that is
invalid.

No: it can throw an exception from the build() method, or
from the SomeWidget() constructor that build() invokes.

I'd be strongly in favour of doing the check in the constructor. That
makes the constructor a sort of final authority on validity, which (a)
means that the validation is done by the class that is, ultimately
validated, and (b) you can ensure that the widgets are valid no matter how
they're made, whether by building or direct construction.

tom
 
A

Andrew

This is a coding bug in the implementation of SomeWidgetBuilder, not
inherent to the pattern. If an attribute that lacks a usable default
value has not been set, why doesn't the build() call throw an exception?

Patricia

I do not like this 'Builder' pattern because even if it was changed in
the the way you say then you still get a runtime error rather than a
compile time error. I prefer to not be able to construct an object
that is invalid. IMO this it is better to detect such errors at
compile time than allowing an invalid object to be created and then
throwing an exception because it is invalid.

One way around this is to give the builder the mandatory arguments
before invoking those 'with' methods I mentioned.

-Andrew Marlow
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

Forum statistics

Threads
473,744
Messages
2,569,483
Members
44,901
Latest member
Noble71S45

Latest Threads

Top