Hello Nicolas, Here are a few inline comments regarding the code.
Maybe I overlooked some good reason justifying some design decision you made, so if you want to discuss more about the suggestions I proposed, we can do some pair programming next week. nma...@apache.org writes: > Author: nmalin > Date: Fri Aug 9 20:37:11 2019 > New Revision: 1864832 > > URL: http://svn.apache.org/viewvc?rev=1864832&view=rev > Log: > Implemented: Homogenize displaying number with multiple format > (OFBIZ-7532) > > To display a number we had different possibilities : > * on ftl use the template <@ofbizAmount and <@ofbizCurrency > * by java call a function UtilFormatOut.formatAmount, > UtilFormatOut.formatPrice, UtilFormatOut.formatQuantity, etc.. > * by form widget, use <display type=accounting-number for accounting but > nothing for other > > To simplify and homogenize all, I implemented a number type purpose : > * default: display a number by default, use when no purpose is present > * quantity: display a number as a quantity > * amount: display a number as an amount (like price without currency) > * spelled: litteral displaying for a number (use on <@ofbizAmount ftl only > before) > * percentage: display a number as a percentage > * accounting: diplay a number for accounting specific > > Each purpose can be associate to a number for displaying it : > * on ftl <@ofbizNumber number=value format=purpose/> > * on java UtilFormatOut.formatNumber(value, purpose, delegator, locale) > * on form widget <display type=number format=purpose/> > > The format use by a purpose is define on > framework/common/config/number.properties with the template > .displaying.format = ##0.00 > > With this, you can surchage a configuration, create your own purpose or > surchage only one through entity SystemProperty. > > Concerning the backware compatibility: > * For the ftl the template <@ofbizAmount is now a link to '<@ofbizNumber > format=amount' > * For java all previous function call UtilFormatOut.formatNumber with the > matching purpose > * For form xml accounting-number is managed as an exection > > Last point, display a currency is different that a number, so I didn't > refactoring some code for this case (only move properties from general to > number for centralize de configuration on the same file) > > Thanks Charles Steltzlen to start the refactoring > [...] > Modified: > ofbiz/ofbiz-framework/trunk/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilFormatOut.java > URL: > http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilFormatOut.java?rev=1864832&r1=1864831&r2=1864832&view=diff > ============================================================================== > --- > ofbiz/ofbiz-framework/trunk/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilFormatOut.java > (original) > +++ > ofbiz/ofbiz-framework/trunk/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilFormatOut.java > Fri Aug 9 20:37:11 2019 [...] > @@ -34,16 +34,11 @@ import java.util.TimeZone; > public final class UtilFormatOut { > > public static final String module = UtilFormatOut.class.getName(); > - > - // ------------------- price format handlers ------------------- > - // FIXME: This is not thread-safe! DecimalFormat is not synchronized. > - private static final DecimalFormat priceDecimalFormat = new > DecimalFormat(UtilProperties.getPropertyValue("general", > "currency.decimal.format", "#,##0.00")); > - > - // ------------------- quantity format handlers ------------------- > - private static final DecimalFormat quantityDecimalFormat = new > DecimalFormat("#,##0.###"); > - > - // ------------------- percentage format handlers ------------------- > - private static final DecimalFormat percentageDecimalFormat = new > DecimalFormat("##0.##%"); > + public static final String DEFAULT_FORMAT = "default"; > + public static final String AMOUNT_FORMAT = "amount"; > + public static final String QUANTITY_FORMAT = "quantity"; > + public static final String PERCENTAGE_FORMAT = "percentage"; > + public static final String SPELLED_OUT_FORMAT = "spelled-out"; Intuitively an ‘enum’ would seem more appropriate to make the relation between those constants clearer. --8<---------------cut here---------------start------------->8--- enum Format { DEFAULT, AMOUNT, QUANTITY, PERCENTAGE, SPELLED_OUT; // Converts a string to a typed value. static Format valueOf(String name) { ... } } --8<---------------cut here---------------end--------------->8--- Additionally I would be nice if the meaning of those “symbols” could be documented inside the code either in the XSD with a reference in the Java code, or in both in XSD and Java. > > private UtilFormatOut() {} > > @@ -54,43 +49,70 @@ public final class UtilFormatOut { > return ""; > } > > - /** Formats a Double representing a price into a string > - * @param price The price Double to be formatted > - * @return A String with the formatted price > + /** Format a number with format type define by properties > + * Please expand the javadoc instead of removing it. > */ > - public static String formatPrice(Double price) { > - if (price == null) { > + public static String formatNumber(Double number, String formatType, > Delegator delegator, Locale locale) { The dependency on the Delegator should be avoided if possible because it makes writing unit complex tests. By the way where are those tests ? :-) To remove a dependency on a class which is hard to instantiate like ‘Delegator’, the common solution is to replace it with an interface (ideally a functional interface to let the caller pass a lambda). In this particular case after a quick look, the ‘delegator’ argument could potentially be replaced by a ‘templateProvider’ argument of type ‘Function<String, String>’ where the input is a format type and the output is a template. When things are complicated to decouple, an alternative is write an overload specifically for the test and use it inside the coupled one. > + if (number == null) { > return ""; > } > - return formatPrice(price.doubleValue()); > + if (formatType == null) { > + formatType = DEFAULT_FORMAT; > + } > + if (locale == null) { > + locale = Locale.getDefault(); > + } Does silencing ‘null’ values really make sense here? if ‘null’ values doesn't have sound meaning, I would recommend bailing out early instead of silencing a possible programming mistake or worse let another method downstream throw a cryptic exception later. To bail out early you can use things like ‘Objects.requireNonNull(myArg)’. Have a nice weekend! -- Mathieu Lirzin GPG: F2A3 8D7E EB2B 6640 5761 070D 0ADE E100 9460 4D37
