why not from(xxx) directly? In any case your proposals sound better for the frenchy I am than of(xxx) ;).
Romain Manni-Bucau @rmannibucau <https://twitter.com/rmannibucau> | Blog <http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> | LinkedIn <https://www.linkedin.com/in/rmannibucau> | Tomitriber <http://www.tomitribe.com> 2015-03-19 13:40 GMT+01:00 Tresch, Anatole <[email protected] >: > I, saw, still an answer is required here ;) > > -----Original Message----- > From: Romain Manni-Bucau [mailto:[email protected]] > Sent: Montag, 16. März 2015 19:32 > To: [email protected] > Subject: Re: ConfigurationBuilder > > 2015-03-16 14:10 GMT+01:00 Werner Keil <[email protected]>: > > > Hi, > > > > The idea of a factory sounds good. I don't like the empty of() since it > > goes against all the current APIs where of() is used, see Java SE 8 > > Date/Time, there is always something like of(argument) or ofMillis(int), > > etc. > > The only notable no-op factory methods are things like now(). > > > > So current() (also in CDI) seems understandable, ofCurrent() disturbing, > > but the idea OF having such static factory methods seems OK to me;-) > > > > Werner > > > > > > > > On Mon, Mar 16, 2015 at 1:55 PM, Anatole Tresch <[email protected]> > > wrote: > > > > > Hi all > > > > > > I have looked at the new builder module a bit more deeper and I just > > write > > > down my suggestions here, so we can discuss them. Some of them also > might > > > be of general interest, regarding how APIs should be built in other > > modules > > > as well: > > > > > > > > > *Constructing a new ConfigurationBuilder:* > > > > > > Currently a user must type new ConfigurationBuilder(); to create a new > > > builder. How about using static factory methods for doing this (in JSR > > 354 > > > user feedback, e.g. from LJC was doing it that way would be preferred). > > As > > > an example, we could define the following: > > > > > > ConfigurationBuilder.of(); // creating a new empty instance > > > ConfigurationBuilder.ofCurrent(); // create a new builder, using the > > > // current configuration as a > > > // starting point, allowing to > > easily > > > // adapt an existing > > configuration... > > > ConfigurationBuilder of(Configuration config); // create a new > builder, > > > // using the given > > > // configuration > > > > > +-0, using an inner class is as common as it AFAIK. That said I hate "of" > for builder, doesnt mean anything for me. > > -> How about create(), createFromCurrentConfig(), > createFrom(Configuration) ? > > > > > *Adding new "items":* > > > > > > There are locations where methods are designed as follows: > > > > > > a) ConfigurationBuilder addItem(Item item, Item... items); > > > b) ConfigurationBuilder addItem(Item item); > > > > > > e.g. ConfigurationBuilder addPropertySource(...). > > > > > > -> Basically the API should be minimalistic, so > > > 1) I assume we can omit method b in that case. > > > 2) Collections (or Iterable) are not supported, so I would think of > > > adding Collection support as > > > well. > > > 2) Since multiple instances could be passed, I would suggest to > name > > > add an 's' to its name. > > > 3) In "Effective Java" the variant with the single item in front is > > > mentioned as an example, > > > where at least one instance must be passed. I personally think > > this > > > variant is > > > overengineered, since we can still either check the input, or > > > simply return (optionally > > > logging a warning). > > > > > > > Why not simply addItems(Item...items) - with or without the "s" I don't > care much > > -> I personally always find it confusing when its singular, but you can > multiple ones, whereas the other way round > a single one mathematically speaking can be seen as a subset of the > multiple ones, which the other way round is > not the case, so I prefer having the 's'... > > > > > Given that I would propose the signature to be looking as: > > > > > > a) ConfigurationBuilder addItems(Item... items); > > > b) ConfigurationBuilder addItems(Collection<Item> items); > > > > > > > -1 to have both - sounds opposed to what you just said, no? > -> 1:0 for you. Keep it simple... > > > > > > > *Boolean Property Signatures:* > > > > > > public boolean isPropertyConverterLoadingEnabled() {...} > > > public ConfigurationBuilder enableProvidedPropertyConverters() {...} > > > public ConfigurationBuilder disableProvidedPropertyConverters() {...} > > > > > > There are multiple boolean flags in the builder (the example ebove > shows > > > one of them). > > > I personally would expect the Java properties style to be applied here, > > so > > > it should be changed to: > > > > > > public boolean isPropertyConverterLoadingEnabled() {...} > > > public ConfigurationBuilder setProvidedPropertyConverters(boolean > > enabled) > > > {...} > > > > > > This reduces the API by 1 method. > > > > > > > For a builder on/off flag methods are common and more expressive. If we go > to setters I'd use setters eveywhere (see my other thread on this) > > -> Matter of taste IMO. I will not insist on this point... > > > > > > > *StringToURLMapper* > > > > > > I would suggest implementing this as a Lambda expression avoiding the > > Java > > > class overhead. If compatibility was the focus with Java 7 it should > not > > > extend java.util.function.Function, because this class is not available > > in > > > Java 7. > > > > > > > +-0 overhead = 0 since lambda implies the same class generation by the JVM > -> As above: I will not insist on this point... > > > > So far my feedback.Let me know what you think! > > > > > > Have a nice day! > > > Anatole > > > > > > > > > -- > > > *Anatole Tresch* > > > Java Engineer & Architect, JSR Spec Lead > > > Glärnischweg 10 > > > CH - 8620 Wetzikon > > > > > > *Switzerland, Europe Zurich, GMT+1* > > > *Twitter: @atsticks* > > > *Blogs: **http://javaremarkables.blogspot.ch/ > > > <http://javaremarkables.blogspot.ch/>* > > > > > > *Google: atsticksMobile +41-76 344 62 79* > > > > > >
