<i18n dev> RFR 8177552: Compact Number Formatting support

Tue Dec 4 18:34:42 UTC 2018

Hi Nishit,

Thanks for the update.  Looks fine.
I have no more comments.

Roger

On 12/04/2018 10:50 AM, Nishit Jain wrote:
> Thanks Roger,
>
> Updated Webrev: 
> http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.06/
>
> Regards,
> Nishit Jain
> On 04-12-2018 00:58, Roger Riggs wrote:
>> Hi Nishit,
>>
>> Thanks for the updates and cleanup.
>>
>> CompactNumberFormat.java:
>>
>> - 827: To locate a single character use:
>>         if (pattern.indexOf(QUOTE) < 0) { ... }
> OK.
>>
>> - 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).
> The computeParseMultiplier decides whether parse string is positive or 
> negative based on the prefix and suffix, so the 
> status[STATUS_POSITIVE] can be also be used to return positive or 
> negative infinity.
ok
>
> Other minor changes in parse():
>
> - Taken out "int numPosition" (line 1444 in webrev.05) and used 
> earlier defined variable "position" instead, as "position" previous 
> value is not used after that statement.
> - Moved the variable "Number multiplier;" (line 1447 in webrev.05) to 
> the place where it is assigned the value.
> - Moved "Number cnfResult;" (line 1496 in webrev.05) inside else block.
>>
>>
>>  - 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.
> Yes, it is better to move it next to "text". Updated both "matchAffix" 
> and "matchPrefixAndSuffix" methods.
>
> Regards,
> Nishit Jain
>>
>> 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 
>> BigDecimal).
>> 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.
>> +1
>>>>
>>>> 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/
>>>>>>
>>>>
>>>
>>
>