<i18n dev> RFR 8177552: Compact Number Formatting support

Roger Riggs Roger.Riggs at oracle.com
Mon Dec 3 19:28:02 UTC 2018

Hi Nishit,

Thanks for the updates and cleanup.


- 827: To locate a single character use:
         if (pattern.indexOf(QUOTE) < 0) { ... }

- 1488:  Since infinite returns do not depend on any of the code after 
line 1454,
    the 1488- 1494 could be moved to 1454. (It is correct where it is).

  - in API design, I would have put the position argument immediately 
after text since the position
    is closely related to the text argument (in matchAffix and 
matchPrefixAndSuffix methods).
    Its probably not worth the change in these private methods.

comments below...

On 12/03/2018 07:22 AM, Nishit Jain wrote:
> Thanks Roger,
> Updated the webrev based on thebelow suggested changes and some clean up.
> http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.05/
> On 01-12-2018 01:03, Roger Riggs wrote:
>> Hi Nishit,
>> Some comments, a couple of possible bugs and several performance 
>> related issues
>> could be deferred. Both formatting and parsing seem to be quite 
>> heavyweight
>> due to looping and combinatorics.
>> CompactNumberFormat.java
>> 661, 727, 1507: Is there a reason to convert the divisor to a string 
>> and then re-parse it to create a new BigDecimal?
>>         (IntelliJ observes that divide is called without a rounding 
>> argument.)
>>         Given that the divisors are all powers of 10 and the digit 
>> list is a base 10 set of digits
>>         it seems more efficient to just move the decimal point then 
>> to do the math.
>>         BTW, the divisor.toString() is preferred over concat with 
>> "".  (looks like a hack).
>>         It would be more efficient to write two methods that would 
>> pass the Number
>>         and return a BigInteger or BigDecimal by dispatching on the 
>> concrete type and
>>         using the appropriate constructor.
> Changed concatenation with toString() and added a rounding parameter, 
> but not getting the benefit of having two methods and returning 
> respective concrete type using constructors.
> I didn't get the point of having two methods. Can you please elaborate?
The would the same function but different return types (BigInteger vs 
The code is ok as is.
>> 781:  @param prefix - the parameter name is suffix
>> 804: move the int start = inside the if.
>> 826:  expandAffix can be more efficient if tests the pattern for the 
>> presence of QUOTE
>>         and returns the pattern if the QUOTE is not present. That 
>> would be the most common case.
>> 914: Reduce the number of compares by reordering to:
>>         if number > currentValue; multiply and continue
>>         if number < currentValue break;
>>         else ==; assign matched index and break;
>>         In the normal case, there will be only one compare in the 
>> loop until it is to exit.
>> 1109:  IntelliJ observes that within the case QUOTE, the if (ch == 
>> QUOTE) is always true
>>       so the if is redundant.
> OK.
>> 1205:  It looks odd to prepend two characters '- to the prefix. Is 
>> the single quote correct?
>>       Where's the closing quote if so.
> It is to specify that the minus sign is a special character, which 
> needs to be replaced with its localized equivalent at the time of 
> formatting.
> Internally, there is a difference between a "minus sign prefix with a 
> single quote" and a "minus sign" (it depends on how user specify the 
> pattern), because the later one is considered as a literal and should 
> be used exactly same in the formatted output, but the one prefixed 
> with a single quote is replaced with its localized symbol using 
> DecimalFormatSymbols.getMinusSign().
thanks for the explanation.
>> 1394: matchedPosPrefix.length() is compared to negativePrefix.length().
>>       That's an unusual mixing of positive and negative! Please 
>> re-check.
> Yes, it was a mistake.
>> 1363:  Can there be an early exit from this loop if one of the 
>> prefixes is identified?
>>       The pattern of comparing for a prefix/suffix and the length 
>> might warrant
>>       creating a private method to reduce the repetitive parts of the 
>> code.
> I had thought about it, but it was difficult unless the whole list of 
> affixes are traversed, because there is always a chance to get longer 
> affix later in the list. I thought to sort the lists based on the 
> length and do the match, but in that case indexes get disordered 
> across lists (divisor, prefix, suffix, compact patterns), and 
> computation will become more complicated.
> Updated the code by moving the repetitive parts in the loop to private 
> methods.
Nice use of an private method to avoid code replication.
>> 1593: extra space between "- ("; should be the same style as 1591
>> 1627, 1363: Is an early exit from this loop possible?
>>        or a quick comparison to avoid the regionMatches.
>>        Do the regionMatches *only if* the prefix/suffix is longer 
>> than the suffix already compared?
> Yes, I think this can be done. Updated.
>> 1721:  IntelliJ observes that if (gotNeg) is redundant since 1708 
>> rules out them being both true or both false.
> Updated

Thanks, Roger

> Regards,
> Nishit Jain
>> Thanks, Roger
>>> On 11/28/18 3:46 AM, Nishit Jain wrote:
>>>> Thanks Naoto,
>>>> Updated.
>>>> http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.04/

More information about the core-libs-dev mailing list