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.
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.
1205: It looks odd to prepend two characters '- to the prefix. Is the
single quote correct?
Where's the closing quote if so.
1394: matchedPosPrefix.length() is compared to negativePrefix.length().
That's an unusual mixing of positive and negative! Please re-check.
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.
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?
1721: IntelliJ observes that if (gotNeg) is redundant since 1708 rules
out them being both true or both false.
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/
