[GitHub] [commons-imaging] darkma773r commented on a change in pull request #116: [IMAGING-159] Add ImagingParameters interface and BaseParameters (POJO)

[GitBox <gi...@apache.org>]

darkma773r commented on a change in pull request #116:
URL: https://github.com/apache/commons-imaging/pull/116#discussion_r680499709



##########
File path: src/main/java/org/apache/commons/imaging/ImageParser.java
##########
@@ -79,25 +78,18 @@
  * that the documentation is perfect, especially in the more obscure
  * and specialized areas of implementation.
  *
- * <h3>The "Map params" argument</h3>
+ * <h3>The "params" argument</h3>
  *
- * Many of the methods specified by this class accept an argument of
- * type Map giving a list of parameters to be used when processing an
- * image. For example, some of the output formats permit the specification
+ * <p>Many of the methods specified by this class accept an argument of
+ * type {@code T} defining the parameters to be used when
+ * processing an image. For example, some of the output formats permit
  * of different kinds of image compression or color models. Some of the
  * reading methods permit the calling application to require strict
- * format compliance.   In many cases, however, an application will not
- * require the use of this argument.  While some of the ImageParser
- * implementations check for (and ignore) null arguments for this parameter,
- * not all of them do (at least not at the time these notes were written).
- * Therefore, a prudent programmer will always supply an valid, though
- * empty instance of a Map implementation when calling such methods.
- * Generally, the java HashMap class is useful for this purpose.
+ * format compliance.</p>
  *
- * <p>Additionally, developers creating or enhancing classes derived
- * from ImageParser are encouraged to include such checks in their code.
+ * @param <T> type of parameters used by this image parser
  */
-public abstract class ImageParser extends BinaryFileParser {
+public abstract class ImageParser<T extends ImagingParameters> extends BinaryFileParser {

Review comment:
       A couple more thoughts here: 
   - I like the idea of having the TIFF and JPEG parameters extend a common ancestor if possible. That seems like a more maintainable solution. With the current hierarchy, if a property needed to be added to the TIFF parameters, it would also be added to the JPEG one, regardless of whether or not it applied to that format.
   - With the removal of the `imageFormat` property, we could completely remove all of the `ImagingParameters` subclasses that do not add any properties, such as `BmpImagingParameters`. They do not seem to add much value anymore and could easily be added later if needed.




-- 
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