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 > > *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). > > Given that I would propose the signature to be looking as: > > a) ConfigurationBuilder addItems(Item... items); > b) ConfigurationBuilder addItems(Collection<Item> items); > > > *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. > > > *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. > > 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* >
