leerho commented on PR #762:
URL:
https://github.com/apache/datasketches-java/pull/762#issuecomment-5576527483
Thank you for finding this. I did a library search and this little function
is used in many places. It is 100% used in test and 100% used in conjunction
with Util.longToFixedLengthString(long number, int length), which means it is
used for printing alignment to make numbers easier to read in columns. Every
case only uses positive numbers, so far.
Nonetheless, you have correctly found a more robust way of computing this
function that works over a wider range of positive numbers. So why stop there?
If we are going to the trouble to fix this function, why not make it even more
useful and allow it to work for negative numbers as well and make the
documentation more clear.
Let's assume the context is printing alignment of simply expressed decimal
numbers with the possibility of a minus sign for negative number, but no
commas, underscores or other special characters. This assumption needs to be
in the javadoc. I would alter your approach to make the algorithm more visible:
```java
/**
* Computes the minimum number of characters required to print the number n
as a decimal.
* Negative numbers add one for the minus sign character.
* No other non-digit characters are assumed.
* @param n the given number, which may be negative.
* @return the number of characters required to print the number n as a
decimal
*/
public static int numDigits(final long n) {
if (n == 0) { return 1; } // handles the zero special case
int count = (n < 0) ? 1 : 0; // handles the minus sign
while (n != 0) {
n /= 10;
count++;
}
return count;
}
```
Your test uses magic numbers, let's not do that:
```java
@Test
static void checkNumDigits() {
for (long n = 1; n < Long.MAX_VALUE && n > 0; n *= 10) {
checkN(n);
checkN(n - 1);
checkN(-n);
checkN(-n + 1);
}
checkN(Long.MAX_VALUE);
checkN(Long.MIN_VALUE);
}
private static void checkN(long n) {
assertEquals(numDigits2(n), String.valueOf(n).length());
}
```
If you want to redo this PR more like the above then go ahead. If not, let
me know and I'll submit a separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
