Marcono1234 commented on code in PR #303:
URL: https://github.com/apache/commons-imaging/pull/303#discussion_r1271477897
##########
src/main/java/org/apache/commons/imaging/ImageFormats.java:
##########
@@ -43,24 +48,29 @@ public enum ImageFormats implements ImageFormat {
XBM("xbm"),
XPM("xpm");
- private final String[] extensions;
+ private final List<String> extensions;
- ImageFormats(final String ...extensions) {
- this.extensions = extensions;
+ ImageFormats(final String... extensions) {
+ this.extensions =
Arrays.stream(extensions).collect(Collectors.toList());
}
@Override
public String getDefaultExtension() {
- return this.extensions != null ? this.extensions[0] : null;
+ return this.extensions != null ? this.extensions.get(0) : null;
}
@Override
- public String[] getExtensions() {
- return this.extensions.clone();
+ public List<String> getExtensions() {
+ return new ArrayList<>(this.extensions);
}
@Override
public String getName() {
return name();
}
+
+
+ public static List<ImageFormats> valuesAsList() {
+ return Arrays.stream(values()).collect(Collectors.toList());
Review Comment:
`Arrays.asList(...)` instead?
And is this new methods even used? If not maybe it should be omitted because
it tries to work around a flaw with the standard Java enum methods (related to
[JDK-8073381](https://bugs.openjdk.org/browse/JDK-8073381)); and fixing this in
a library might not be the right place?
##########
src/main/java/org/apache/commons/imaging/ImageFormats.java:
##########
@@ -43,24 +48,29 @@ public enum ImageFormats implements ImageFormat {
XBM("xbm"),
XPM("xpm");
- private final String[] extensions;
+ private final List<String> extensions;
- ImageFormats(final String ...extensions) {
- this.extensions = extensions;
+ ImageFormats(final String... extensions) {
+ this.extensions =
Arrays.stream(extensions).collect(Collectors.toList());
}
@Override
public String getDefaultExtension() {
- return this.extensions != null ? this.extensions[0] : null;
+ return this.extensions != null ? this.extensions.get(0) : null;
}
@Override
- public String[] getExtensions() {
- return this.extensions.clone();
+ public List<String> getExtensions() {
+ return new ArrayList<>(this.extensions);
Review Comment:
What about returning an unmodifiable list here (respectively directly making
the list unmodifiable in the `ImageFormats` constructor)? It appears multiple
parsers store a single instance of the result of this and then expose it
through their `getAcceptedExtensions()` method, so this could lead to
accidental modification.
##########
src/main/java/org/apache/commons/imaging/ImageFormats.java:
##########
@@ -43,24 +48,29 @@ public enum ImageFormats implements ImageFormat {
XBM("xbm"),
XPM("xpm");
- private final String[] extensions;
+ private final List<String> extensions;
- ImageFormats(final String ...extensions) {
- this.extensions = extensions;
+ ImageFormats(final String... extensions) {
+ this.extensions =
Arrays.stream(extensions).collect(Collectors.toList());
Review Comment:
Why not `java.util.Arrays.asList(...)`? `Collectors.toList()` makes not
guarantees about the created list, see
[documentation](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/stream/Collectors.html#toList()):
> There are no guarantees on the type, mutability, serializability, or
thread-safety of the List returned
If the intention here is to create a copy of the array, you could also do
`Arrays.asList(extensions.clone())`; that would make the intention clearer then.
(But I don't think there is a need to copy the array here.)
##########
src/main/java/org/apache/commons/imaging/formats/pnm/PnmImageParser.java:
##########
@@ -42,13 +43,13 @@
public class PnmImageParser extends ImageParser<PnmImagingParameters> {
private static final String DEFAULT_EXTENSION =
ImageFormats.PNM.getDefaultExtension();
- private static final String[] ACCEPTED_EXTENSIONS = {
- ImageFormats.PAM.getDefaultExtension(),
- ImageFormats.PBM.getDefaultExtension(),
- ImageFormats.PGM.getDefaultExtension(),
- ImageFormats.PNM.getDefaultExtension(),
- ImageFormats.PPM.getDefaultExtension()
- };
+ private static final List<String> ACCEPTED_EXTENSIONS = Arrays.asList(
Review Comment:
Should this additionally wrap the list in `Collections.unmodifiableList`?
Because while the list returned by `Arrays.asList` does not support changing
the size, it does still support replacing elements.
--
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]
