On Fri, 15 Dec 2023 14:09:12 GMT, Eirik Bjorsnos <[email protected]> wrote:
>> Archie Cobbs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Address review comments.
>
> test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 52:
>
>> 50: buf.reset();
>> 51: for(int i = 0; i < 100; i++)
>> 52: buf.write(gz);
>
> Whitespace after `for`, braces are recommended even when optional in the
> language:
>
> Suggestion:
>
> for (int i = 0; i < 100; i++) {
> buf.write(gz);
> }
Thanks - should be addressed in 074b8455b11.
> test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 53:
>
>> 51: for(int i = 0; i < 100; i++)
>> 52: buf.write(gz);
>> 53: final byte[] gz32 = buf.toByteArray();
>
> Drop final, consider finding a more expressive name:
>
> Suggestion:
>
> byte[] concatenatedGz = buf.toByteArray();
Thanks - should be addressed in 074b8455b11.
> test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 56:
>
>> 54:
>> 55: // (a) Read it back from a stream where available() is accurate
>> 56: long count1 = countBytes(new GZIPInputStream(new
>> ByteArrayInputStream(gz32)));
>
> This is the expected number of bytes to be read. This could be calculated
> directly from the uncompressed data. At least the name should express that
> this is the expected number of bytes:
>
> Suggestion:
>
> long expectedBytes = countBytes(new GZIPInputStream(new
> ByteArrayInputStream(gz32)));
>
>
> Similarly, `count2` could be renamed `actualBytes`
Thanks - should be addressed in 074b8455b11.
> test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 62:
>
>> 60:
>> 61: // They should be the same
>> 62: Assert.assertEquals(count2, count1);
>
> This could be a static import. And for the assertion failure message to be
> correct, the expected value must come first:
>
> Suggestion:
>
> assertEquals(expectedBytes, actualBytes);
>
>
> An alternative here could be to store the original uncompressed data and
> compare that to the full inflated data read from GZIPInputStream using
> assertArrayEquals. The length alone is a bit of a weak proxy for equality.
Thanks - should be addressed in 074b8455b11.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428174716
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428175505
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428174894
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428175182
