On Thu, 26 Jan 2023 05:30:56 GMT, Glavo <d...@openjdk.org> wrote:
> `List.of` is cleaner, and can slightly reduce the memory footprint for lists
> of one or two elements.
>
> Because `List.of` can only store non-null elements, I have only replaced a
> few usage.
>
> Can someone open an Issue on JBS for me?
### FileChooser
`List.of` and `List.copyOf` already check for a `null` argument and `null`
elements. This means that `validateArgs` only needs to check the `length` of
`extensions` and for an empty element. Since the only difference between the
constructors is taking an array vs. taking a list, once a list is created from
the array, the array constructor can call the list constructor.
I suggest the following refactoring:
public ExtensionFilter(String description, String... extensions) {
this(description, List.of(extensions));
}
public ExtensionFilter(String description, List<String> extensions) {
var extensionsList = List.copyOf(extensions);
validateArgs(description, extensionsList);
this.description = description;
this.extensions = extensionsList;
}
Note that `List.copyOf` will not create another copy if it was called from the
array constructor that already created an unmodifiable `List.of`.
Now validation can be reduced to
private static void validateArgs(String description, List<String>
extensions) {
Objects.requireNonNull(description, "Description must not be null");
if (description.isEmpty()) {
throw new IllegalArgumentException("Description must not be
empty");
}
if (extensions.isEmpty()) {
throw new IllegalArgumentException("At least one extension must
be defined");
}
for (String extension : extensions) {
if (extension.isEmpty()) {
throw new IllegalArgumentException("Extension must not be
empty");
}
}
}
Aditionally, please add empty lines after the class declarations of
`FileChooser` and `ExtensionFilter`.
Also please correct the typo in `getTracks`: "The returned <code>List</code>
**is** unmodifiable."
modules/javafx.graphics/src/main/java/com/sun/javafx/iio/common/ImageDescriptor.java
line 41:
> 39: this.extensions = List.of(extensions);
> 40: this.signatures = List.of(signatures);
> 41: this.mimeSubtypes = List.of(mimeSubtypes);
While this is not equivalent since changing the backing array will not change
the resulting list anymore, I would consider the old code a bug and this the
correct fix.
Note that in `FileChooser` care is taken to create a copy of the supplied array
before using it to create a list.
Additionally, care must be taken that all the callers don't have `null`
elements in the arrays they pass. I checked it and it's fine (and should
probably be disallowed, which is good now).
By the way, this class should be a `record`, and all its subtypes, which are
not really subtypes but just configured instances, should be modified
accordingly. This is out of scope though.
modules/javafx.media/src/main/java/com/sun/media/jfxmedia/Media.java line 100:
> 98: returnValue = null;
> 99: } else {
> 100: returnValue = List.copyOf(tracks);
This is fine because `addTrack` checks for `null` elements.
modules/javafx.media/src/main/java/com/sun/media/jfxmedia/Media.java line 103:
> 101: }
> 102: }
> 103: return returnValue;
This method can be reduced to
public List<Track> getTracks() {
synchronized (tracks) {
return tracks.isEmpty() ? null : List.copyOf(tracks);
}
}
though I find it highly questionable that it returns `null` for an empty list
instead of just an empty list. There are 2 use cases of this method and both
would do better with just an empty list.
-------------
Changes requested by nlisker (Reviewer).
PR: https://git.openjdk.org/jfx/pull/1012
