23 ms·

The simple, readable, loop-based snippet mentioned in the article also works for the edge cases. Sometimes it's better to not be so clever.

Ragib_Zaman 7y ago
And the pow and log methods they call run loops anyway. What is wrong with loops anyway?
- JacksCracked 7y ago
  Even more worrisome is that the OP came up with a clever way to solve the problem but didn't bother to leave a comment on the code explaining how it works. As a result, everyone that copied the code, did so without any comments as well.
- Recursing 7y ago
  > the pow and log methods they call run loops anyway. That's actually not true (as somebody mentioned on reddit) https://github.com/openjdk-mirror/jdk/blob/jdk8u/jdk8u/master/src/share/native/java/lang/fdlibm/src/e_log.c https://github.com/openjdk-mirror/jdk/blob/jdk8u/jdk8u/maste... It is probably slower anyway, but I was surprised to see no loops
  - rand_r 7y ago
    They kind of cheated because they used a hard-coded polynomial approximation of log(x). So there is an implicit unrolled loop going over A_i*x^i.
Diggsey 7y ago
No, it doesn't. It has the exact same rounding bug described in the article. The article even mentions: > FWIW, all 22 answers posted, including the ones using Apache Commons and Android libraries, had this bug (or a variation of it) at the time of writing.
- jandrese 7y ago
  That seems a bit confusing given how the loop works. 999,999 shouldn't be big enough to be affected by floating point rounding. If the comment is correct then Java will evaluate ( 999999 >= 1000000 ) as true, which simply cannot be correct.
  - Diggsey 7y ago
    You're misunderstanding the bug. In the end, the number will be displayed to one decimal place, eg. "1.1 MB" or similar due to the "%.1f" format specifier. If the input is 999999 bytes, the loop will see that it is less than 1 MB and so will format the number 999.999 into "%.1f kB". When this number is rounded to one decimal place as part of that formatting, it rounds up to "1000.0 kB". This is the wrong output. There's a bit of a catch 22 because ideally, you would be able to do the rounding first, and then see what units need applying, but you can't do the rounding until you know what the divisor will be. The article solves this by first manually determining what the cut-off should be (it will be some number like "...999500...") but personally I'd probably just decide to round to significant figures instead so that you can cleanly separate the "rounding" and "unit selection" steps.
    - jandrese 7y ago
      Oh, that makes sense, although I'm not sure "1000.0 kB" is strictly wrong in this case. With the loop at least it's easy to adjust the thresholds if that is desirable, although a comment will be necessary to explain why you are making the cutoffs in weird places. Interestingly enough in the past I've used a loop similar to this: static char suffix[] = { ' ', 'k', 'm', 'g', 't' }; magnitude = 0; while ( value > 1000 ) { value /= 1000; magnitude++; } printf("%.1f%cB", value, suffix[magnitude]); Which is bad because it's subject to repeated rounding from the division, but avoids the problem you described.
      - LorenPechtel 7y ago
        I don't think the multiple divisions will do any harm as you only need 4 digits of precision left at the end.

Ragib_Zaman 7y ago

And the pow and log methods they call run loops anyway. What is wrong with loops anyway?

JacksCracked 7y ago
Even more worrisome is that the OP came up with a clever way to solve the problem but didn't bother to leave a comment on the code explaining how it works. As a result, everyone that copied the code, did so without any comments as well.
Recursing 7y ago
> the pow and log methods they call run loops anyway. That's actually not true (as somebody mentioned on reddit) https://github.com/openjdk-mirror/jdk/blob/jdk8u/jdk8u/master/src/share/native/java/lang/fdlibm/src/e_log.c https://github.com/openjdk-mirror/jdk/blob/jdk8u/jdk8u/maste... It is probably slower anyway, but I was surprised to see no loops
- rand_r 7y ago
  They kind of cheated because they used a hard-coded polynomial approximation of log(x). So there is an implicit unrolled loop going over A_i*x^i.

Diggsey 7y ago

No, it doesn't. It has the exact same rounding bug described in the article. The article even mentions: > FWIW, all 22 answers posted, including the ones using Apache Commons and Android libraries, had this bug (or a variation of it) at the time of writing.

jandrese 7y ago
That seems a bit confusing given how the loop works. 999,999 shouldn't be big enough to be affected by floating point rounding. If the comment is correct then Java will evaluate ( 999999 >= 1000000 ) as true, which simply cannot be correct.
- Diggsey 7y ago
  You're misunderstanding the bug. In the end, the number will be displayed to one decimal place, eg. "1.1 MB" or similar due to the "%.1f" format specifier. If the input is 999999 bytes, the loop will see that it is less than 1 MB and so will format the number 999.999 into "%.1f kB". When this number is rounded to one decimal place as part of that formatting, it rounds up to "1000.0 kB". This is the wrong output. There's a bit of a catch 22 because ideally, you would be able to do the rounding first, and then see what units need applying, but you can't do the rounding until you know what the divisor will be. The article solves this by first manually determining what the cut-off should be (it will be some number like "...999500...") but personally I'd probably just decide to round to significant figures instead so that you can cleanly separate the "rounding" and "unit selection" steps.
  - jandrese 7y ago
    Oh, that makes sense, although I'm not sure "1000.0 kB" is strictly wrong in this case. With the loop at least it's easy to adjust the thresholds if that is desirable, although a comment will be necessary to explain why you are making the cutoffs in weird places. Interestingly enough in the past I've used a loop similar to this: static char suffix[] = { ' ', 'k', 'm', 'g', 't' }; magnitude = 0; while ( value > 1000 ) { value /= 1000; magnitude++; } printf("%.1f%cB", value, suffix[magnitude]); Which is bad because it's subject to repeated rounding from the division, but avoids the problem you described.
    - LorenPechtel 7y ago
      I don't think the multiple divisions will do any harm as you only need 4 digits of precision left at the end.