[GitHub] [commons-imaging] Marcono1234 commented on a diff in pull request #303: IMAGING-322: ImageFormat uses a list for extensions instead of array

["Marcono1234 (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org