kinow commented on a change in pull request #155:
URL: https://github.com/apache/commons-imaging/pull/155#discussion_r663321550
##########
File path:
src/main/java/org/apache/commons/imaging/formats/jpeg/decoder/JpegDecoder.java
##########
@@ -242,43 +242,41 @@ public boolean visitSegment(final int marker, final
byte[] markerBytes,
} else if (marker == JpegConstants.DQT_MARKER) {
final DqtSegment dqtSegment = new DqtSegment(marker, segmentData);
for (final QuantizationTable element :
dqtSegment.quantizationTables) {
- final DqtSegment.QuantizationTable table = element;
Review comment:
Or we could rename `element` to `table`? WDYT @arturobernalg ? This way
the "Invalid quantization table identifier" message would clearly reflect that
the identifier is from `table` (as in the variable name too).
Table is also a common term in many image processing documents (I guess
that's why the author of the code created the alias-variable).
##########
File path:
src/main/java/org/apache/commons/imaging/formats/rgbe/RgbeImageParser.java
##########
@@ -102,15 +102,14 @@ public BufferedImage getBufferedImage(final ByteSource
byteSource, final Map<Str
final DataBuffer buffer = new DataBufferFloat(info.getPixelData(),
info.getWidth() * info.getHeight());
- final BufferedImage ret = new BufferedImage(new
ComponentColorModel(
+ return new BufferedImage(new ComponentColorModel(
ColorSpace.getInstance(ColorSpace.CS_sRGB), false, false,
Transparency.OPAQUE, buffer.getDataType()),
Raster.createWritableRaster(
new BandedSampleModel(buffer.getDataType(),
info.getWidth(), info.getHeight(), 3),
buffer,
new Point()), false, null);
- return ret;
Review comment:
:+1:
##########
File path:
src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/DataReaderTiled.java
##########
@@ -85,26 +85,24 @@ private void interpretTile(final ImageBuilder imageBuilder,
final byte[] bytes,
if (sampleFormat ==
TiffTagConstants.SAMPLE_FORMAT_VALUE_IEEE_FLOATING_POINT) {
// tileLength: number of rows in tile
// tileWidth: number of columns in tile
- final int i0 = startY;
Review comment:
Didn't search any specification document, but I would probably leave
these variables as-is as they appear to be the type of vars copied from some
existing code or document...
##########
File path:
src/main/java/org/apache/commons/imaging/formats/png/PngImageParser.java
##########
@@ -254,9 +254,8 @@ public void readSignature(final InputStream is) throws
ImageReadException,
}
final PngChunkIccp pngChunkiCCP = (PngChunkIccp) chunks.get(0);
- final byte[] bytes = pngChunkiCCP.getUncompressedProfile(); // TODO
should this be a clone?
- return (bytes);
+ return (pngChunkiCCP.getUncompressedProfile());
Review comment:
This removes the `TODO` comment that was left there by Sebb during one
of his reviews. Better leave it there until someone else has time to ponder on
that item, and maybe remove the redundant variable.
##########
File path:
src/main/java/org/apache/commons/imaging/formats/jpeg/decoder/JpegDecoder.java
##########
@@ -376,16 +374,14 @@ private void readMCU(final JpegInputStream is, final
int[] preds, final Block[]
is,
huffmanACTables[scanComponent.acCodingTableSelector]);
final int ssss = rs & 0xf;
- final int rrrr = rs >> 4;
- final int r = rrrr;
Review comment:
It does look odd, but if you look at the comments near this code, they
are pointing to which page of a document the author was referencing when
writing it. These variable names, though obscure or redundant, are more helpful
for devs familiar with that document.
See this Python examples:
-
https://github.com/Exaphis/python-jpeg-decoder/blob/849c3d0bb32355e44a40364ea0498e6f59443716/jpeg.py#L302
-
https://github.com/yohhoy/picojdec/blob/316ef275c757bb67d4398980660f42915f67901c/picojdec.py#L152
Or these excerpts from a specification document:
I use Eclipse + IntelliJ (and other tools like CPD, SpotBugs, stan4j,
sonarqube, etc) but for [imaging] I ignore several issues to avoid reducing the
clarity of the code for future developers familiar with image processing :+1:
So sorry for turning down changes like this @arturobernalg , even though you
are correct some variables are redundant.
--
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-unsubscr...@commons.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
