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
cleaned up the new aztec decoder by @iSDP referenced in issue #381 #583
Conversation
} | ||
|
||
public static byte[] convertBoolArrayToByteArray(boolean[] boolArr) { | ||
byte[] byteArr = new byte[boolArr.length / 8]; |
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.
Is it certain that the array's length will be a multiple of 8? otherwise this would not work.
Have you tested the result to see that it matches what you expect? Maybe piggy-backing a check for this result on some of the existing tests would be a good idea.
Finally, add braces for all if and for statcements.
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 certainly can add the missing braces and resubmit, but I don't feel comfortable changing the actual code, as it's a bit over my head. Maybe @isdp can weigh in on this or add the checks you're talking about.
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.
At least throw an exception if this condition isn't true, otherwise some data is silently ignored. Add at least one test and this is good to merge.
if (array[start + i]) { | ||
b |= 1 << (7 - i); | ||
} else { | ||
throw new IndexOutOfBoundsException(); |
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.
What's this about? it certainly isn't an error
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 feel @isdp and I have hit a dead end here, as we both don't know enough Java to fix this or throw a decent error. Until somebody figures this out, Or I learn enough, there is nothing I can do. Sorry about the trouble and thanks for listening.
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.
Ok. Just mean I am not sure why you added this change. It is wrong and makes the test fail. I suggested checking the length of the array. Containing false is not the same thing nor an error. It also needs a simple test to demonstrate and verify the expected behavior
Just in case you didn't notice, I wrote a fix for this. I'm afraid I forgot to mention your PR in mine. #591 |
I'm so happy we can put this to rest. Thank you for helping out! |
A while back @isdp complained that the Aztec decoder does not return raw bytes like eg. the QR decoder. He made a PR #389, but it was rejected due to formatting issues.
I'd really like for this to work. Many Aztec barcodes don't store string data but compressed zlib streams, which get corrupted when turned into strings. The raw bytes would help.
I have not written any of the lines in this PR, only reformatted @isdp's work to match the repo's conventions. I hope this is ok.