leerho commented on PR #762:
URL:
https://github.com/apache/datasketches-java/pull/762#issuecomment-5655335024
Interesting. GitHub Advanced Security identified a condition that is always
true, but it identified (n > 0), the wrong one!
In the expression:
`for (long n = 1; n < Long.MAX_VALUE && n > 0; n *= 10) {}`
It is the (n < Long.MAX_VALUE) that is always true not the (n > 0). So this
expression could be simplified to:
`for (long n = 1; n > 0; n *= 10) {}` // When rollover occurs it initially
goes negative, which halts the loop.
I should have caught that, my bad.
Nonetheless, I still prefer this simple loop over what you did and here is
why:
YOU know that 10^18 is the largest multiple of 10 that doesn't exceed
Long.MAX_VALUE. So you set maxExp = 18, again a magic value. So the success of
your loop depends on this magic value.
It is preferable to not depend on magic values if we don't have to, because
magic values can become incorrect if something else in this little algorithm
changed ... like if _n_ was defined as a 32 bit signed integer. Magic numbers
can be fragile.
Instead let's use intrinsic properties of the language: when a signed
positive integer rolls over it becomes negative. This is independent of whether
_n_ is defined as a byte, short, int or long.
And what is this loop from -1000 to 1000 all about? Are you concerned that
the main loop will miss a value that will fail?
If you closely examine the actual sequence of values of _n_ from the main
loop you will discover the sequence:
_n_ = 1, 0, -1, 0, 10, 9, -10, -9, 100, 99, -100, -99, 1000, 999, -1000,
-999 ...
In the decimal system, the points where the number of decimal characters
change are the transitions:
-1 <-> 0, 9 <-> 10, -10 <-> -9, 100 <-> 99, -100 <-> -99, etc.
If we know our algorithm works at all the transition points, it is easy to
show that it will work for all the numbers in between. The only transition
left is the Long.MAX_VALUE, Long.MIN_VALUE, which is also tested.
So I would prefer if you change the test method back to what I suggested
before, minus the redundant condition that I missed:
```
/**
* Check all the transition points where the number of decimal characters
change.
*/
@Test
static void checkNumDigits() {
for (long n = 1; n > 0; n *= 10) { // n goes negative on rollover, which
halts the loop.
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());
}
```
--
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]
