-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Udpate PngModule.java #587
Conversation
Updating fork from base repository
Constant FORMAT[0] set as "PNG"; made 2 replacements of literal "PNG" with FORMAT[0] literal string Unused declared twice in PNG_PROFILES[], delete one Unused Constant PNG_PROFILES[] declared, replaced literal strings of png profiles with constant
Codecov Report
@@ Coverage Diff @@
## integration #587 +/- ##
==============================================
Coverage 45.63% 45.63%
Complexity 1046 1046
==============================================
Files 57 57
Lines 9149 9149
Branches 1687 1687
==============================================
Hits 4175 4175
Misses 4424 4424
Partials 550 550 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a couple of points to consider, for me at least, but the approval isn't in any way tentative. Thanks.
@@ -379,7 +378,7 @@ public final int parse(final InputStream inputStream, | |||
IdentifierType.URL)); | |||
_specification.add (doc); | |||
|
|||
Signature sig = new InternalSignature ("PNG", SignatureType.MAGIC, | |||
Signature sig = new InternalSignature (FORMAT[0], SignatureType.MAGIC, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just becoming aware that this approach does introduce an issue, although it's still an improvement. FORMAT[0]
isn't as easy to read, but using a single source is a good idea. There's a couple of solutions come to mind but to establish consistency across the codebase I'll need to consider some other files and cases. This is fine for now as it weeds out the repetition.
@@ -961,7 +960,7 @@ private final void checkIHDR(final DataInputStream inputStream, | |||
colorType + ": " +tmp )); | |||
|
|||
} | |||
repInfo.setProfile("PNG GrayScale"); | |||
repInfo.setProfile(PNG_PROFILES[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From here onwards are all improvements regardless as the strings are so long. I suspect that there's a better way of setting these up in the first place using enums but that's beyond the scope of this work and this is the first step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Carl. Java is still new to me so if you think of a better way to address these categorized strings, let me know. Going forward should I continue to set up constants this way (for instance, there are some repeated error messages in this file that I am thinking of using the enums Error_Message[] for, but should just use a separate constant for each error message?)
Addresses issue #573
Constant FORMAT[0] set as "PNG"; make 2 replacements of literal "PNG" with FORMAT[0]
Literal string "Unused" declared twice in PNG_PROFILES[], delete one "Unused"
Constant PNG_PROFILES[] declared, replace 5 literal strings of png profiles with appropriate PNG_PROFILES[] constant.