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

Check if characters are digits for EAN_13, EAN_8, ITF and UPC_EAN_EXTENSION #1039

Merged
merged 7 commits into from Jul 10, 2018

Conversation

Projects
None yet
3 participants
@keoni29
Contributor

keoni29 commented Jun 30, 2018

Several barcode formats only support digits 0-9, namely

  • BarcodeFormat.EAN_13
  • BarcodeFormat.EAN_8
  • BarcodeFormat.ITF
  • BarcodeFormat.UPC_EAN_EXTENSION

Passing different characters to the encode function causes a runtime exception.

Proposed solution
Check return value of Character.digit and throw IllegalArgumentException if it is -1 (invalid character.)
Source: https://developer.android.com/reference/java/lang/Character#digit(int,%20int)

@codecov-io

This comment has been minimized.

codecov-io commented Jun 30, 2018

Codecov Report

Merging #1039 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1039      +/-   ##
============================================
+ Coverage     76.17%   76.23%   +0.06%     
- Complexity     4200     4204       +4     
============================================
  Files           252      252              
  Lines         14010    14018       +8     
  Branches       2871     2871              
============================================
+ Hits          10672    10687      +15     
+ Misses         2538     2531       -7     
  Partials        800      800
Impacted Files Coverage Δ Complexity Δ
...src/main/java/com/google/zxing/oned/ITFWriter.java 76% <100%> (+1%) 6 <0> (ø) ⬇️
...c/main/java/com/google/zxing/oned/EAN13Writer.java 77.14% <100%> (+6.55%) 7 <0> (ø) ⬇️
...rc/main/java/com/google/zxing/oned/UPCEWriter.java 70.58% <100%> (+6.95%) 7 <0> (ø) ⬇️
...om/google/zxing/oned/OneDimensionalCodeWriter.java 82.85% <100%> (+2.21%) 14 <3> (+3) ⬆️
...rc/main/java/com/google/zxing/oned/EAN8Writer.java 74.19% <100%> (+7.52%) 6 <0> (ø) ⬇️
.../main/java/com/google/zxing/oned/UPCEANReader.java 82.96% <0%> (+0.74%) 35% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cf3b9b...1bff106. Read the comment docs.

@srowen

This comment has been minimized.

Contributor

srowen commented Jun 30, 2018

Rather than check every call to Character.digit, how about checking the input once at the start of the various encode methods with a regex? You can make a utility method in UPCEANWriter maybe.

private static final Pattern NUMERIC = Pattern.compile("[0-9]+");
...
if (!NUMERIC.matcher(contents).matches()) {
  ...
} 

Lastly if you have a moment to add a simple test of these behaviors that would really complete it.

@@ -42,4 +42,14 @@ public void testAddChecksumAndEncode() throws WriterException {
assertEquals(testStr, BitMatrixTestCase.matrixToString(result));
}
@Test
public void testEncodeIllegalCharacters() throws WriterException {

This comment has been minimized.

@srowen

srowen Jul 1, 2018

Contributor

You should be able to write @Test(expected = WriterException.class) and then the body of the test is just the line that causes the exception.

This comment has been minimized.

@keoni29

keoni29 Jul 8, 2018

Contributor

I expect an IllegalArgumentsException, so I test for this instead.

@@ -90,6 +91,7 @@ private static BitMatrix renderResult(boolean[] code, int width, int height, int
return output;
}
private static final Pattern NUMERIC = Pattern.compile("[0-9]+");

This comment has been minimized.

@srowen

srowen Jul 1, 2018

Contributor

Can you write a simple checkNumeric method here or something and then just call it in each of the subclasses? that way the invocation isn't duplicated.

@srowen

This comment has been minimized.

Contributor

srowen commented Jul 5, 2018

@keoni29 if you'll make the last updates, I'll merge

@keoni29

This comment has been minimized.

Contributor

keoni29 commented Jul 6, 2018

Have not had time this week to look at it. I can probably get it done this weekend. Thanks for your suggestions.

keoni29 added some commits Jul 7, 2018

@keoni29

This comment has been minimized.

Contributor

keoni29 commented Jul 8, 2018

Should be all right now.

@@ -90,6 +92,9 @@ private static BitMatrix renderResult(boolean[] code, int width, int height, int
return output;
}
protected static boolean checkNumeric(String contents) {
return NUMERIC.matcher(contents).matches();

This comment has been minimized.

@srowen

srowen Jul 8, 2018

Contributor

Why not just have this method check whether it matches and throw the exception? that way you don't have to repeat the check and throw everywhere. It can be final too.

*/
protected static final void checkNumeric(String contents) throws IllegalArgumentException{
if (!NUMERIC.matcher(contents).matches()) {
throw new IllegalArgumentException("Input should only contain digits 0-9");

This comment has been minimized.

@srowen

srowen Jul 10, 2018

Contributor

The indentation is off here and in a test below but maybe I can just adjust it later. Change looks OK.

This comment has been minimized.

@keoni29

keoni29 Jul 10, 2018

Contributor

Resolved with latest commit

@srowen srowen merged commit 48bb5fd into zxing:master Jul 10, 2018

4 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch 100% of diff hit (target 76.17%)
Details
codecov/project 76.23% (+0.06%) compared to 0cf3b9b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

makiuchi-d added a commit to makiuchi-d/gozxing that referenced this pull request Sep 5, 2018

antimony added a commit to antimony/zxing that referenced this pull request Oct 1, 2018

Check if characters are digits for EAN_13, EAN_8, ITF and UPC_EAN_EXT…
…ENSION (zxing#1039)

* Check if characters are digits

* Use regex to check if input is valid. Added tests

* Create checkNumeric method

* Remove unused locals, moved attributes to top of class.

* Simplify testcase

* Reduce repeated code

* Fixed indentation

@srowen srowen added the enhancement label Nov 9, 2018

@srowen srowen added this to the 3.3.4 milestone Nov 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment