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/
