<i18n dev> RFR 8177552: Compact Number Formatting support
Roger Riggs
Roger.Riggs at oracle.com
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/
>>>>>>
>>>>
>>>
>>
>
More information about the core-libs-dev
mailing list