[
https://issues.apache.org/jira/browse/IMAGING-159?focusedWorklogId=625590&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-625590
]
ASF GitHub Bot logged work on IMAGING-159:
------------------------------------------
Author: ASF GitHub Bot
Created on: 20/Jul/21 13:16
Start Date: 20/Jul/21 13:16
Worklog Time Spent: 10m
Work Description: gwlucastrig commented on a change in pull request #116:
URL: https://github.com/apache/commons-imaging/pull/116#discussion_r673100545
##########
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:
Here's an idea for a plan (or at least 12 percent of an plan):
In the package example.common, we have the class ExampleParameters and the
interface ExampleParser
```
public class ExampleParameters {
public int test;
}
public interface ExampleParser {
public void parse(ExampleParameters paramaters);
}
```
There are multiple implementing format packages. In package
example.format.alpha, we have
```
public class AlphaParameters extends ExampleParameters {
int mess;
/**
* A copy constructor
* @param source
*/
AlphaParameters(ExampleParameters source){
this.mess =source.test;
}
}
public class AlphaParser implements ExampleParser {
@Override
public void parse(ExampleParameters parameters) {
AlphaParameters alphaParameters;
if(parameters instanceof AlphaParameters){
alphaParameters = (AlphaParameters)parameters;
}else{
alphaParameters = new AlphaParameters(parameters);
}
parse(alphaParameters);
}
public void parse(AlphaParameters parameters){
// do the work
}
}
```
My thinking is that most applications that have format-specific parameters,
will call their parser class directly and pass in the specific class that goes
with it (in this example, AlphaParameter... in real life TiffImagingParameters,
etc.). Even those that passed in a generic would experience a small overhead
for the downcast or the copy constructor... Those application that are use the
high-level ImageParser class (which determines format and branches based on
format type) would pass in an instance of the base class ExampleParameters.
After all, it's only a small performance hit and if they really have
format-specific needs (as I do in my applications), they would instantiate a
format-specific parser.
This approach has the advantage of avoiding Generics, which some developers
might find challenging or obscure, especially in this particular case.
Also, the copy constructor that I used in this example wouldn't always be
necessary. A parser class could also do something like:
```
@Override
public void parse(ExampleParameters parameters) {
if (parameters instanceof AlphaParameters) {
processAlphaParameters((AlphaParameters) parameters);
} else {
processGeneralParameters(parameters);
}
```
Whether we use a copy constructor or a branch as shown above, my overall
approach does have one disadvantage: it doesn't detect misuse of the API for
all contingencies at compile time. For example, if we had
```
// BetaParameters extends ExampleParameters
BetaParameters betaParameters = new BetaParameters();
AlphaParser alphaParser = new AlphaParser();
alphaParser.parse(betaParameters);
```
This would compile just fine. However, betaParameters would contain
information that was irrelevant to the alpha parser. It would probably be a
case where the programmer made a mistake. The program would probably run, but
it might not do quite what the developer hoped. Some debugging would be
necessary. How could we help this case? We could introduce run-time checking
into all of our parsers, or we could just let it go.
I'm leaning to the "just let it go option", but here's one way that the
run-time checking could be accomplished. In the AlphaParser class, we have
```
if (parameters instanceof AlphaParameters) {
alphaParameters = (AlphaParameters) parameters;
} else {
enforceUseOfBaseClass(parameters);
alphaParameters = new AlphaParameters(parameters);
}
```
And the method:
```
void enforceUseOfBaseClass(ExampleParameters parameters){
if(!ExampleParameters.class.equals(parameters.getClass())){
throw new IllegalArgumentException("parameters is not the base
class");
}
}
```
--
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]
Issue Time Tracking
-------------------
Worklog Id: (was: 625590)
Time Spent: 7h 20m (was: 7h 10m)
> There should be a Parameters class
> ----------------------------------
>
> Key: IMAGING-159
> URL: https://issues.apache.org/jira/browse/IMAGING-159
> Project: Commons Imaging
> Issue Type: Improvement
> Components: imaging.*
> Affects Versions: 1.0-alpha2
> Reporter: Benedikt Ritter
> Assignee: Bruno P. Kinoshita
> Priority: Major
> Labels: github
> Fix For: 1.0-alpha3
>
> Time Spent: 7h 20m
> Remaining Estimate: 0h
>
> Currently options for image I/O are defined as Maps. The leads to the problem
> that our code has to validate parameter types when they are used:
> {code:java}
> final Object value = params.get(PARAM_KEY_COMPRESSION);
> if (value != null) {
> if (!(value instanceof Number)) {
> throw new ImageWriteException(
> "Invalid compression parameter, must be numeric: "
> + value);
> }
> compression = ((Number) value).intValue();
> }
> {code}
> This can be simplified if we define a Parameters class that provides
> additional methods like {{public int getInt(String key)}}. The implementation
> could then look up the value from the map through an exception if it is null
> or not a number.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
