On 8/10/19 12:07 AM, Mathieu Lirzin wrote:
Hello Nicolas,
Hi man,
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.
It was a big task, so I focused my mind only on how homogenize and
centralize a number displaying. So if you I have some idea to continue
the improvement through other design concept, It's welcome :)
[...]
@@ -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.
Indeed
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.
I will check, maybe it's an error
*/
- 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 ? :-)
Normally not because delegator and locale are optional. If it's not the
case, I will improve it to support unit test
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.
I's seem to be a good idea :) but I'm far away to understand what do you
explain, I need to studies more ^^
+ 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)’.
I use the silencing because we are on displaying number so instead
deploy complex analyze with risk to break something during the
rendering, I prefer to use safe call en just return empty String.
Have a nice weekend!
Thanks for your time and remarks
Nicolas
