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