+1 IMO, for optimization. When you have a performance issue, it rarely happens because you created small arrays by use of ellipse operators. The GC will care about them ;) On top if that the arrays are typically not empty and creating an array is relatively cheap compared to create a collection type...
BTW: I don’t want to start a discussion on performance issues etc. ;) -----Original Message----- From: Romain Manni-Bucau [mailto:[email protected]] Sent: Dienstag, 17. März 2015 09:03 To: [email protected] Subject: Re: ConfigurationBuilder Well about optim I think that is one of the place we care the less since you'll use it not often and I am not even sure you can notice it at all. Keeping API simpler is better to start. Le 17 mars 2015 08:31, "Oliver B. Fischer" <[email protected]> a écrit : > Hi, > > where is the advantage of a factory method if the result of this method is > always of the same type as the class providing the factory method? Static > factory methods look cool and modern but I do not like them as there is no > advantage at the moment in using them for this class. > > Concerning adding new items. Providing these overloaded method helps the > compiler to optimize the generated code as it selects the best fit for the > method call and helps to avoid temporary objects. > > Bye, > > Oliver > > > > Am 16.03.15 um 13:55 schrieb Anatole Tresch: > >> 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 >> >> >> > -- > N Oliver B. Fischer > A Schönhauser Allee 64, 10437 Berlin, Deutschland/Germany > P +49 30 44793251 > M +49 178 7903538 > E [email protected] > S oliver.b.fischer > J [email protected] > X http://xing.to/obf > >
