Skip to content

Conversation

@mmindenhall
Copy link

Hello,
Very cool library! I have run across lots of binary message formats that include binary coded decimal (aka packed decimal)...including one I need to support on my current project. So I spent a few hours yesterday and most of today adding BCD support (signed and unsigned) as a field type in the parsing DSL. I added tests for everything I did, and the existing tests are all passing. I hope you'll consider accepting this contribution!

This is not quite complete, as I don't yet have support for BCD using your @Bin annotation. I just ran out of time today, and need to work on something else the rest of the week. I can finish it up later, but would appreciate some pointers on what needs to be done.

@raydac
Copy link
Owner

raydac commented May 20, 2015

Nice code and idea! thank you very much!
I am very glad that you like the library and use it.
The Main issue with the pull request is that you have taken the last value of compiler (0x0F), it is nice to add packed BCD into process but how to be if a user needs non-packed BCD or fixed point float BCD? or something else? it is interesting use case, I will think how to improve architecture to support your case and give a way to another use cases too

@mmindenhall
Copy link
Author

  1. Yes, I noticed that I took the last value in the compiler range...but it seemed like it would not be too difficult to expand that range to allow for additional future types?
  2. The utility class is non-static because it's easier to test. E.g., if you ever need to write a test for a class that uses the utility class, you might want to mock the utility class (e.g., using mockito or spock), and that's much harder to do with static methods. In general, I try to avoid static methods as much as possible, and use Guice to create and inject services as singletons. Since this is a library and it would not be good to have external dependencies on things like Guice, I just created a class that gets instantiated when used. It could be made into a singleton, or the methods could be made static if you prefer.
  3. I would have used a logger instead of System.out if one had been available. What do you think about adding SLF4J logging into the project? Then consumers of the library can decide what underlying logging library and logging levels they want to use. I guess most developers would figure it out eventually, by looking through the code and/or Javadoc, but in a library such as this I think it's good to have logging.
  4. I used string representation as the intermediate phase because BCD has to do with the number of decimal digits, and not the bit size (64-bit signed) of the long. For example, for a signed BCD representation using 2 bytes, one nibble is reserved for the sign, which means the numeric part can have a maximum of 3 digits -- i.e., values can range from -999 to 999. I don't know of another way in Java to get the number of decimal digits other than converting to a string and counting the characters (after removing the "-" sign if present).

@mmindenhall
Copy link
Author

Another thought regarding future expansion...I think there are some additional types that would be convenient to have. One thing lacking is support for strings within the binary data blocks. That should be pretty easy to do, but the question is what to support? Maybe there could be a char type in the DSL, with a separate setting (similar to byte order) used to specify the character set?

That also leads to a potential refactoring for specifying settings as additional capabilities are added. For example, instead of a bunch of prepare(...) methods with different combinations of settings, perhaps a Builder pattern would work better to override default settings?

  JBBPParser parser = JBBPParser.builder(script)
      .bitOrder(JBBPBitOrder.MSB0)
      .byteOrder(JBBPByteOrder.LITTLE_ENDIAN)
      .packedDecimalType(JBBPPackedDecimalType.SIGNED)
      .charSet(StandardCharsets.UTF_16)
      .build();

  JBBPFieldStruct result = parser.parse(theBytes);

When default settings are used, that would become:

  JBBPParser parser = JBBPParser.builder(script).build();
  JBBPFieldStruct result = parser.parse(theBytes);

@raydac
Copy link
Owner

raydac commented May 20, 2015

the library should work and on mobile platforms so I don't want to add links to external libraries and logging, messages about wrong behaviour should be provided to user through exceptions
I have improved API to support custom field types so now it is possible define external processors for different types without increase library size, take a look at the example, I adapted some your tests and unpacking function

@mmindenhall
Copy link
Author

Cool...I like the solution for adding custom field types! We'll also need to add support for strings, and we can do that by implementing JBBPCustomFieldTypeProcessor. I also like the aggregator approach to combining multiple custom processors. Please go ahead and close this pull request, and I'll move ahead with this new approach for BCD.

I had also been working on a separate pull request (just rebased from your new changes) for creating a Map<String, Object> from the JBBPFieldStruct after parsing. This is needed to easily convert the parsed data to another representation. For example, we need to convert to JSON, and most JSON libraries can read from a Map<String, Object>. There are also libraries to convert between Map and XML.

This is an essential capability for how I'm intending to use the library. Is this something you'd be willing to include, or should I focus on building my own utility class to walk a JBBPFieldStruct and its children to produce the Map?

@raydac
Copy link
Owner

raydac commented May 21, 2015

in my opinion all what can be formed as extra library should not be included into core because it will provide possibility to users to load and use only what they need and will be working and in mobile devices and in enterprise environment, extra libraries can be oriented to different platforms and include logging and anything else without limit of size, just write me if you need some architecture improvements without which it is impossible to go ahead

@raydac raydac closed this May 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants