On Fri, 29 Nov 2024 16:13:42 GMT, Chen Liang <li...@openjdk.org> wrote:
>> Please review this PR which applies various cleanups to
>> `sun.net.www.protocol.file.FileURLConnection`.
>>
>> This class is known to be an _old, intricate, and hard to maintain piece of
>> code._ However, there are some relatively straightforward refactorings /
>> cleanups possible which improve readability and makes it easier to reason
>> about what's going on in this class.
>>
>> In this PR, I have chosen to make each individual small change a separate
>> commit. This to assist review of each individual change, which can otherwise
>> disappear a bit when reviewing the PR as a whole.
>>
>> A detailed listing of each commit follows in a separate comment.
>>
>> This is a pure cleanup PR, no tests are added or updated in this PR.
>
> src/java.base/share/classes/sun/net/www/protocol/file/FileURLConnection.java
> line 78:
>
>> 76: if (fileList == null)
>> 77: throw new FileNotFoundException(file.getPath() + "
>> exists, but is not accessible");
>> 78: directoryListing = Arrays.<String>asList(fileList);
>
> Is the `<String>` parameterization necessary? I tested on jshell, and it
> seems since the destination variable's type is `List<String>` we don't need
> this explicit parameterization to distinguish varargs.
Yes, indeed this can be dropped. See 06b2a23.
> src/java.base/share/classes/sun/net/www/protocol/file/FileURLConnection.java
> line 193:
>
>> 191: String fileName = directoryListing.get(i);
>> 192: sb.append(fileName);
>> 193: sb.append("\n");
>
> We can use a `StringJoiner` or stream to join the strings instead. (Note
> there is a suffix `\n` in the strings)
`StringJoiner` will not include the trailing line break. I want to be
conservative in this PR so reluctant to introduce streams for this.
Opted to use an enhanced for-loop. See f9efa3b.
EDIT: Aha, `StringJoiner`, not `String.join` :-) Still, I think I prefer
keeping the explicit iteration for this PR.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22459#discussion_r1863827672
PR Review Comment: https://git.openjdk.org/jdk/pull/22459#discussion_r1863827700
