Re: svn commit: r1864832 - in /ofbiz/ofbiz-framework/trunk: applications/accounting/config/ applications/order/template/order/ applications/order/widget/ordermgr/ framework/base/src/main/java/org/apache/ofbiz/base/util/ framework/common/config/ framework/w...

[Mathieu Lirzin]

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.

[hidden email] 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

[Nicolas Malin-2]

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

>

[Mathieu Lirzin]

Nicolas Malin <[hidden email]> writes:

> On 8/10/19 12:07 AM, Mathieu Lirzin wrote:
>
>> 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 :)

This is indeed a big effort which is adding a great amount of
flexibility.

>>>        */
>>> -    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 me this is the only critical part that needs to be fixed because
previous implementation was achieving better testability. Other remarks
are less important improvements that can be worked on later.

>> 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 ^^

Decoupling methods from stateful objects like ‘Delegator’,
‘HttpServletrequest’ and ‘GenericValue’ is hard to explain and
understand theorically, however in practice when adopting the hygiene of
writing unit tests with plain java objects it become second nature. :-)

Thanks.

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37