-
Notifications
You must be signed in to change notification settings - Fork 782
consistent builder pattern #5929
Comments
I really like your proposal to harmonize all of our builders. All of the above suggestions make sense to me, but I fear that changing our builders is a huge API breaking change, because they are used everywhere. Newly added builders like my |
I would add another additional requirement to the builder, to require in the constructor or create method the mandatory fields, those without which the object does not make sense |
I had this already in my mind and don't know if this is what the builder pattern is used for. Let's assume you have three mandatory fields of the same type, e.g. The constructor of the builder will consume this three arguments. Hasen't been the builder patter been used to make it more readable? FooBuilder(a,b,c).withBar(4).build(); vs. FooBuilder().withX(a).withY(b).withZ(c).withBar(4).build(); the latter one is IMHO more readable. Also, if I check if "a", "b" or "c" is known / non-null or if I catch e.g. an What do you think about a checked exception (e.g. |
yes it is but all the best practices say:
see for example the book "Effective java 2nd edition" - Joshua Bloch |
We start using the builder pattern on more places in the ESH code base.
IMHO the current implementations do not follow a consistent and intuitive behavior.
See #5911 (comment) and following comments.
Here a short summary that fits to the most implementations (some needs to be changed):
with...
with a reference to something that is not a collection (single value)set the value
don't add something to a collection if multiple members are possible but replace
with...s
with a collectioncreate an internal collection that contains all the given members
overwrite the existing collection, so don't add but replace
add...
with a reference to something that is not a collection (single value)add the value to an internal collection, don't replace the current members
with...
for collections and non-collections: handlenull
argumentthe internal representation should be unset, which could be handled differently, e.g.
set internal null
set internal a default value
Different implementations use different behavior (AFAIK) -- and if already all implementations use "that" (the one we agree later) scheme, we should use that scheme for all new implementation.
I will create a Wiki entry for our builder pattern in the ESH code if we agree...
IMHO it does not make sense if
withFoos
replace the current collection with a new one using the provided collection's members, butwithFoo
add the given argument to the members. With should be set or add but not the one for collections and the other one for non-collections. It should not make any difference if the caller useswithFoo(foo)
orwithFoos(Collections.singleton(foo))
.As we don't know if the builder is used at one place or used at different ones in before
build
is called, we should not ignorenull
values IF the value is markednullable
. Let's assume the builder is used at some place where the caller wants to remove a perhaps previously set value. If a method signals thatnull
is allowed I would expect that it does something (e.g. reset the value to null or default), it should not be ignored silently. If you don't want to provide some reset function, don't allow a nullable member.The text was updated successfully, but these errors were encountered: