<i18n dev> RFR 8177552: Compact Number Formatting support
nishit.jain at oracle.com
Wed Nov 21 08:53:16 UTC 2018
Updated the webrev based on suggestions
- Replaced List<String> with String to be added to the the resource
- refactored DecimalFormat.subparse() to be used by the CNF.parse(), to
reduce code duplication.
- Also updated it with other changes as suggested in the comments
On 20-11-2018 00:33, naoto.sato at oracle.com wrote:
> Hi Nishit,
> On 11/18/18 10:29 PM, Nishit Jain wrote:
>> Hi Naoto,
>> Please check my comments inline.
>> On 17-11-2018 04:52, naoto.sato at oracle.com wrote:
>>> Hi Nishit,
>>> Here are my comments:
>>> - CLDRConverter: As the compact pattern no more employs
>>> List<String>, can we eliminate stringListEntry/Element, and use
>>> Array equivalent instead?
>> Since the CNF design does not put any limit on the size of compact
>> pattern, so at the time of parsing the CLDR xmls using SAX parser, it
>> becomes difficult to identify the size of array when the parent
>> element of compact pattern is encountered, so I think it is better to
>> keep the List<String> while extracting the resources.
> OK. However I'd not keep the List<String> format on generating the
> resource bundle, as there is no reason to introduce yet another bundle
> format other than the existing array of String.
>>> - CompactNumberFormat.java
>>> Multiple locations: Use StringBuilder instead of StringBuffer.
>>> line 268: The link points to NumberFormat.getNumberInstance(Locale)
>>> instead of DecimalFormat
>> OK. Changed it at line 165 also.
>>> line 855: no need to do toString(). length() can detect whether it's
>>> empty or not.
>>> line 884: "Overloaded method" reads odd here. I'd prefer
>>> specializing in the "given number" into either long or biginteger.
>>> line 1500: subparseNumber() pretty much shares the same code with
>>> DecimalFormat.subparse(). can they be merged?
>> The existing CNF.subParseNumber differs in the way parseIntegerOnly
>> is handled, DecimalFormat.parse()/subparse() behaviour is
>> unpredictable with parseIntegeronly = true when multipliers are
>> involved (Please see JDK-8199223).
>> Also, I had thought that the CNF.parse()/subparseNumber() should *not
>> *parse the exponential notation e.g. while parsing "1.05E4K" the
>> parsing should break at 'E' and returns 1.05, because 'E' should be
>> considered as unparseable character for general number format pattern
>> or compact number pattern, but this is not the case with
>> DecimalFormat.parse(). The below DecimalFormat general number format
>> NumberFormat nf = NumberFormat.getNumberInstance();
>> Successfully parse the string and returns 10500. The same behaviour
>> is there with other DecimalFormat instances also e.g. currency instance.
>> Do you think this is an issue with DecimalFormat.parse() and CNF
>> should avoid parsing exponential numbers? Or, should CNF.parse() be
>> modified to be consistent with DecimalFormat.parse() in this aspect?
> No, I understand there are differences. But I see a lot of duplicated
> piece of code which I would like to eliminate.
>>> line 1913-1923, 1950-1960, 1987-1997, 2024-2034: It simply calls
>>> super. No need to override them.
>> Since setters are overridden, I think that it is better to override
>> getters also (even if they are just calling super and have same
>> javadoc) to keep them at same level. But, if you see no point in
>> keeping them in CNF, I will remove them. Does that need CSR change?
> I don't see any point for override. I don't think there needs a CSR,
> but better ask Joe about it.
>>> line 2231: You need to test the type before cast. Otherwise
>>> ClassCastException may be thrown.
>> The type is checked in the superclass equals method getClass() !=
>> obj.getClass(), so I think there is no need to check the type here.
>> Nishit Jain
>>> On 11/16/18 9:54 AM, Nishit Jain wrote:
>>>> Please review this non trivial feature addition to NumberFormat API.
>>>> The existing NumberFormat API provides locale based support for
>>>> formatting and parsing numbers which includes formatting decimal,
>>>> percent, currency etc, but the support for formatting a number into
>>>> a human readable or compact form is missing. This RFE adds that
>>>> feature to format a decimal number in a compact format (e.g. 1000
>>>> -> 1K, 1000000 -> 1M in en_US locale) , which is useful for the
>>>> environment where display space is limited, so that the formatted
>>>> string can be displayed in that limited space. It is defined by
>>>> LDML's specification for Compact Number Formats.
>>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8177552
>>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8188147
>>>> Request to please help review the the change.
>>>> Nishit Jain
More information about the i18n-dev