Skip to content
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

Closed
wants to merge 3 commits into from

Conversation

oelna
Copy link

@oelna oelna commented Apr 22, 2016

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.

}

public static byte[] convertBoolArrayToByteArray(boolean[] boolArr) {
byte[] byteArr = new byte[boolArr.length / 8];
Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

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();
Copy link
Contributor

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

Copy link
Author

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.

Copy link
Contributor

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

@tanelihuuskonen
Copy link
Contributor

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

@oelna
Copy link
Author

oelna commented May 9, 2016

I'm so happy we can put this to rest. Thank you for helping out!
(I'm actually hoping this change makes it into ZXingObjC eventually) 😊

@srowen srowen closed this May 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants