garydgregory commented on code in PR #467:
URL: https://github.com/apache/commons-imaging/pull/467#discussion_r1861337355
##########
src/main/java/org/apache/commons/imaging/common/BigEndianBinaryOutputStream.java:
##########
@@ -22,29 +22,38 @@
final class BigEndianBinaryOutputStream extends BinaryOutputStream {
+ private static final int MASK_8_BITS = 0xff; // Magic number 0xff
explained
+ private static final int BYTE_SHIFT_8 = 8; // Magic number 8 explained
+ private static final int BYTE_SHIFT_16 = 16; // Magic number 16 explained
+ private static final int BYTE_SHIFT_24 = 24; // Magic number 24 explained
+
BigEndianBinaryOutputStream(final OutputStream os) {
super(os);
}
@Override
public void write2Bytes(final int value) throws IOException {
- write(0xff & value >> 8);
- write(0xff & value);
+ writeByteWithShift(value, BYTE_SHIFT_8); // Extracted and reused logic
+ writeByteWithShift(value, 0);
}
@Override
public void write3Bytes(final int value) throws IOException {
- write(0xff & value >> 16);
- write(0xff & value >> 8);
- write(0xff & value);
+ writeByteWithShift(value, BYTE_SHIFT_16);
Review Comment:
This is not better IMO, it makes the code even more verbose, and Java's
already pretty verbose ;-) The current version make it very clear that we are
doing bit level manipulation.
##########
src/main/java/org/apache/commons/imaging/formats/dcx/DcxImageParser.java:
##########
@@ -172,7 +173,7 @@ private DcxHeader readDcxHeader(final ByteSource
byteSource) throws ImagingExcep
public void writeImage(final BufferedImage src, final OutputStream os,
final PcxImagingParameters params) throws ImagingException, IOException {
final int headerSize = 4 + 1024 * 4;
- final BinaryOutputStream bos = BinaryOutputStream.littleEndian(os);
+ final BinaryOutputStream bos = BinaryOutputStreamFactory.create(os,
ByteOrder.LITTLE_ENDIAN);
Review Comment:
This is much worse IMO. You're trading a clear factory method with 1 arg for
another with 2. Worse, the PR allows for null input where none makes sense, and
then would throw an exception anyway. The current code does not suffer from
this confusion.
##########
src/main/java/org/apache/commons/imaging/common/BigEndianBinaryOutputStream.java:
##########
@@ -22,29 +22,38 @@
final class BigEndianBinaryOutputStream extends BinaryOutputStream {
+ private static final int MASK_8_BITS = 0xff; // Magic number 0xff
explained
+ private static final int BYTE_SHIFT_8 = 8; // Magic number 8 explained
+ private static final int BYTE_SHIFT_16 = 16; // Magic number 16 explained
+ private static final int BYTE_SHIFT_24 = 24; // Magic number 24 explained
+
BigEndianBinaryOutputStream(final OutputStream os) {
super(os);
}
@Override
public void write2Bytes(final int value) throws IOException {
- write(0xff & value >> 8);
- write(0xff & value);
+ writeByteWithShift(value, BYTE_SHIFT_8); // Extracted and reused logic
+ writeByteWithShift(value, 0);
}
@Override
public void write3Bytes(final int value) throws IOException {
- write(0xff & value >> 16);
- write(0xff & value >> 8);
- write(0xff & value);
+ writeByteWithShift(value, BYTE_SHIFT_16);
+ writeByteWithShift(value, BYTE_SHIFT_8);
+ writeByteWithShift(value, 0);
}
@Override
public void write4Bytes(final int value) throws IOException {
- write(0xff & value >> 24);
- write(0xff & value >> 16);
- write(0xff & value >> 8);
- write(0xff & value);
+ writeByteWithShift(value, BYTE_SHIFT_24);
+ writeByteWithShift(value, BYTE_SHIFT_16);
+ writeByteWithShift(value, BYTE_SHIFT_8);
+ writeByteWithShift(value, 0);
}
-}
+ // Extracted method to remove duplication
+ private void writeByteWithShift(final int value, final int shift) throws
IOException {
+ write(MASK_8_BITS & (value >> shift));
+ }
+}
Review Comment:
File should end in a blank line. GitHub shows you this here with a red icon.
--
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]
