Nice comments! Let me focus on something to explain. Regarding merge AbstractAsn1Type and Asn1Encodeable. As I said before, the former wraps a Java value, the latter does the encoding/decoding dirty work. The separation may does no good so far, but would also do no hurt for the left. Something may start with the latter and doesn't like the generic thing. How about if we make the parser results encodeable/decodeable? On other Java platform that doesn't support the generic thing very well? How about if we bridge this library to other ASN1 stuffs? Yes as you said these are just maybe, uncertain things. But if the merging doesn't generate some good for now, I thought it may be not bad to go the way.
I guess you're mentioning two approaches: either let the ASN1 objects do encode/decode themselves, or have some help class to delegate the dirty work out. Yeah this is an architecture decision that's done when coming up the first piece of the codes. I remembered I tried many times, but never got a way that works perfect in every aspect. We used the former approach that goes not bad so far if we don't attempt to support many other coding rules and do BER/DER as good as possible. But if we would do, then we may want to simplify the ASN1 objects and need to delegate the encoding/decoding things out to helper/facet classes. I don't investigate the latter direction very much, to be frankly. One thing in mind would be, let the library be driven by our applications. If it's required and needs to solve some new cases, then evolve the library to get the new cases done elegantly. I would admit that developing the library itself is much a burden for us because our limited effort should be focused on Kerberos specific, but as you see, we've spent very much time on this, though I believe it's worth because it can make Kerberos side easier with it. Yeah Asn1Specifix should be Asn1Specific, good catch. Currently it's very simple and doesn't do much work. Will see if any further cases that require it to do more work. A reason for this is, the systems so far using the library are majorly type/model driven encoding/decoding from top to bottom, which makes the library behave very differently from others. The major work for application/context specific types are already handled elsewhere. We need it for Asn1.decodeAndDump() call. Regards, Kai -----Original Message----- From: Emmanuel Lécharny [mailto:[email protected]] Sent: Thursday, December 24, 2015 5:03 PM To: [email protected] Subject: Re: [kerby] ASN1 Quick review Le 24/12/15 01:56, Zheng, Kai a écrit : > Nice picture!! My comments are embedded marked as [Kai]. > > -----Original Message----- > From: Emmanuel Lécharny [mailto:[email protected]] > Sent: Thursday, December 24, 2015 1:29 AM > To: Apache Directory Developers List <[email protected]> > Subject: Re: [kerby] ASN1 Quick review > > Some more questions, now that I have drawn the full Asn1 hierarchy > (http://pasteboard.co/fGVRQFm.png) > [Kai] Thanks a lot. Very cool! Could we integrate it in the library doc? Of course ! Find the attached graphml file (the source from which I generated the picture, using yEd - https://www.yworks.com/products/yed) > > - can't we merge the Asn1Encodable and AbstractAsn1Type classes ? > [Kai] I guess we can, actually before AbstractAsn1Type is very large. I would > prefer to separate them becaue the roles of them are clearly different. Separation of concern is actually better done with Interfaces. > Asn1Encodable is responsible for all the encoding/decoding dirty works; and > AbstractAsn1Type would wrap a Java type value using Java generic. The real question here is more or less an architctural question. Let's consider that an object might have to support multiple 'facets' (as in http://i.cs.hku.hk/~clwang/projects/facet.html) - should we let a Class implement a 'facet' ? - or should we delegate this facet to an helper class ? For encodable, I would rather think that the help class approach would be way too complex, as each ASN1 class has its own implementation of some of teh Asn1Encodable classes. This make me think that the first approach is better. Now, as all the Asn1 classes that extend AbstractAsn1Type already have their own implementations of methods like encodingLength(), I'd rather see the AbstractAsn1Type abstract class implement what is currently in the Asn1Encodable class, and make Asn1Encodable an Interface (becaise the other branch of Asn1Object aren't implementing it). > For now all Asn1Type object are AbstractAsn1Type, but may be not in the > future, because there are possible situations where some types don't want to > start with AbstractAsn1Type using the generic things. DO you have some types in mind ? Or is this just a door you want to maintain open, just in case ? > I wish the library could evolve further so be able to standalone as a > complete solution some day. Sure, but I think it's preferable to have a clear vision and design at each step, which can evolve in the future. > > - same question for the Asn1Cnstructed and Asn1Collection classes > [Kai] Again they were together before but separated out recently. > Asn1Collection is purely for set/setoff/sequence/sequenceof. Asn1Constructed > can be another case where primitive type but using constructed encoding. You > could check the places where it's used. An example in Asn1Converter. Here, that makes more sense. > > - I'm not sure the Asn1Dumpable interface makes a lot of sense. Why don't we > simply implement a toString() method that takes an identation as a parameter ? > [Kai] Right initially when implementing the dump support we went the way, we > used a separate method toStr() but found it doesn't work nicely. toString() > with a indent parameter looks anti-intuitive I guess. A dumpable can be > passed a dumper as well and using the dumper it can share the same underlying > String builder along with all kinds of utility functions. Note only complex > and bridge types like Asn1CollecionType, Asn1Any and Asn1Container need to go > the way, simple types use Java toString() naturally. Forget what I suggested. It's clear that we *need* this interface, it forces the classes to implement the methods, otehrwise we might forget doing so, ending with an incomplete 'dump' feature. Asn1Specifix (which name should probably be Asn1Specifics) should implement this interface, btw. Thanks !
