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*
