consistent builder pattern

[maggu2810]

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 https://github.com/eclipse/smarthome/pull/5911#discussion_r202941725 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 collection create 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: handle null argument the 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, but withFoo 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 uses withFoo(foo) or withFoos(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 ignore null values IF the value is marked nullable. 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 that null 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.

[triller-telekom]

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 ChannelTypeBuilder could still be changed (if we are quick) before anyone uses them, but I do not know about the others ones...

[adimova]

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

[maggu2810]

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

The constructor of the builder will consume this three arguments. The order of the arguments depends on the current order of the method declaration.

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 BuildException of the build method does not enlarge the code base.

What do you think about a checked exception (e.g. BuildException) that can be thrown by the build method (if there are mandatory fields) and so (because of "checked" exception) must be catch by the caller?

[adimova]

the latter one is IMHO more readable

yes it is but all the best practices say:

Instead of making the desired object directly, the client calls a constructor (or static factory) with all of the required parameters and gets a builder object. Then the client calls setter-like methods on the builder object to set each optional parameter of interest. Finally, the client calls a parameterless build method to generate the object, which is immutable.

see for example the book "Effective java 2nd edition" - Joshua Bloch