Resend after formatted. Thanks Emmanuel for the review and great comments! The questions are hard but fortunately I'm still kept in the loop so my pleasure to address them. My comments are embedded and marked by [Kai].
-----Original Message----- From: Emmanuel Lécharny [mailto:elecha...@gmail.com] Sent: Wednesday, December 23, 2015 8:54 PM To: Apache Directory Developers List <dev@directory.apache.org> Subject: [kerby] ASN1 Quick review Hi band, I'm having a quick look at the kerby-asn1 code, as I wanted to paly around the idea of porting the LDAP codec to use this piece of nice code. AFAIU, when you want to declare an object that can be encoded or decoded, you have to extend the correct Asn1Object child. Looking at the Ticket object, here is what I see : [Kai] Glad you want a try. This is also something I wish to help with as discussed before, but am not able to do it immediately because of bandwidth. Sorry for that. I certainly would and will also be able to try to provide any help if needed. ... import static org.apache.kerby.kerberos.kerb.type.ticket.Ticket.MyEnum.*; ... public class Ticket extends KrbAppSequenceType { public static final int TKT_KVNO = KrbConstant.KRB_V5; public static final int TAG = 1; protected enum MyEnum implements EnumType { TKT_VNO, REALM, SNAME, ENC_PART; @Override public int getValue() { return ordinal(); } @Override public String getName() { return name(); } } static Asn1FieldInfo[] fieldInfos = new Asn1FieldInfo[] { new ExplicitField(TKT_VNO, 0, Asn1Integer.class), new ExplicitField(REALM, 1, KerberosString.class), new ExplicitField(SNAME, 2, PrincipalName.class), new ExplicitField(ENC_PART, 3, EncryptedData.class) }; I like the idea of defining the fields this way, except that I would suggest some slight modifications : - get read of the import ...Ticket.MyEnum.*; [Kai] Hmmm, I guess you mean "get rid of", good point. It's like this because, initially we didn't use enum, but int constants. And some weeks ago when we want to dump user defined type objects like this with meaningful field info, we switched to use enum because it can give a friendly name. We were in a hurry at that time and wanted to do it as less effort as possible, thus it's like this now: the enum constant is rather like int constant and used as before. - make the enum private (there is no reason we wuld like to expose it anyway) [Kai] Well, actually there are some reasons to make it protected and disciplined exposed. Such user defined types can be extended and these fields may be accessed there. Ref. child classes of ContentInfo (also some other examples I remembered when defining protected int constants). - name it something more meaningful, like TicketField inseatd if MyEnum [Kai] Right agree. Again it was like this because it's out as a simple pattern in a whole project scope replacement. - use it with the enum name, like in new ExplicitField(TicketField.TKT_VNO, 0, Asn1Integer.class) [Kai] Agree. <side note> I find the "import static xxx.*;" atrocious, most of the time. It makes it barely readable : you have to go to the top of your file to actually *know* where the constant comes from... That may seems of for small files, but otherwise... I accept it for Asserts, because it's really clear, in a test context - pretty much like annotations. </side note> [Kai] I thought you may be right, but ... Here is what I come with : public class Ticket extends KrbAppSequenceType { public static final int TKT_KVNO = KrbConstant.KRB_V5; public static final int TAG = 1; private enum TicketField implements EnumType { TKT_VNO, REALM, SNAME, ENC_PART; @Override public int getValue() { return ordinal(); } @Override public String getName() { return name(); } } static Asn1FieldInfo[] fieldInfos = new Asn1FieldInfo[] { new ExplicitField(TicketField.TKT_VNO, 0, Asn1Integer.class), new ExplicitField(TicketField.REALM, 1, KerberosString.class), new ExplicitField(TicketField.SNAME, 2, PrincipalName.class), new ExplicitField(TicketField.ENC_PART, 3, EncryptedData.class) }; (note that it's just a suggestion at this point...) [Kai] They're very good suggestions and should become true, though a tiresome work because there are so many user defined types now. I'm wondering if any contributor would help with such. Now, let's foduc on the fields declarations. Here, we create ExplicitFields, passing a Xxx.class as the third parameter, and a number as the second parameter. First of all, as the enum being used has an implicit ordering, can't we simply avoid passing those numbers ? There is already an ExplicitField constructor that takes only 2 parameters, deducing the number from teh enum... [Kai] Yeah sure we can just use the 2 parameters constructor for this type. You're good at finding this as the example. We relied on the enum order value mostly but this one and possible many slipped out. Second, why don't we write things like : new ExplicitField(TicketField.TKT_VNO, 0, new Asn1Integer()), instead of passing a class ? Bottom line, the instance will be created the same way. I don't think it will make any difference in term of performances, and as all the object *must* extends the Asn1Object (or implement the Asn1Type), this should be good enough. [Kai] Well, let the framework determine when to create the field instances would be most flexible. We may want to be lazy on creating them and create them on demand; we may need to avoid creating them in decoding because they're nulls; in the framework codes there're some places that uses (value == null) to check some conditions. For Any, the actual value type can only be set by applications in specific contexts. wdyt ?
