Skip to content

Adding 5.0 version of barcode scanner #3

Closed
wants to merge 8 commits into from

5 participants

@jeffheifetz

No description provided.

@tneil
Open Source Projects member
tneil commented Aug 15, 2011

Hi Jeff,

Can we also engage Ken with the inclusion of the Zebra X'ings libraries. I believe he has started this work as well for the Tablet OS.

Just making sure we have all the right t's crossed and i's dotted.

@ababut
ababut commented Aug 24, 2011

Jeff,

This looks good, and complete, if a little busy. My comments are related largely to modularizing the code a bit better to improve readability and maintenance.

General nitpicks:

  • please check the line spacing as it is inconsistent in all files
  • when checking a variable against a constant, use CONSTANT.equals(variable) to avoid NPEs

Specific comments:
1) BarcodeExtension - there shouldn't be two if statements checking the same thing in the loadFeature method. I also recommend breaking down the code that checks and sets permissions into two helper methods (getMissingCameraPermissions and allowMissingCameraPermissions) to improve readability.

2) GenerateBarcodeAction - I think that the invoke method can be broken down into several pieces to improve readability. I suggest keeping the the parameter checking in invoke and assigning the parameters to the contents, generatedCallback, errorCallback and options objects directly as you're checking them. The runnable can be a separate private class. Invoke can process, break down and send the options object to the class' constructor to make the dependencies clear.

Its run method does several things - processes args, converts a string representation of the barcode to a byte array and processes files to write it. I recommend separating those steps into smaller methods so that the run method reads like pseudo-code. The helpers can take care of the details for whoever is interested in scrolling down and digging deeper.

3) ScanBarcodeAction - same comments as above. It's one busy invoke method that can be broken down to improve readability.

@astanley

Checking to see if this pull request simply fell off the table, or if there is a reason why it has not yet been merged.

It looks like Jeff addressed Alex's concerns. Is there anything else left to do before we can add this change?

@pelegri
pelegri commented Jan 30, 2013

Pull request no longer merges cleanly. I'll ping offline to see if we can close this, or whether @jeffheifetz should update it

@pelegri
pelegri commented Jan 30, 2013

Closing after talking with @astanley

@pelegri pelegri closed this Jan 30, 2013
@t470520 t470520 added a commit to t470520/WebWorks-Community-APIs that referenced this pull request Oct 29, 2014
@t470520 t470520 #3 sameple files update again
remove all LICENCE file, replace with plain text files. Update index.js
3374d19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.