Re: [Pki-devel] [PATCH] 0001-First-cut-at-Java-TPS-Buffer-class-and-APDU-class.patch

Endi thanks for the comments, resolutions below: ----- Original Message ----- > From: "Endi Sukma Dewata" <edewata(a)redhat.com&gt; > To: "John Magne" <jmagne(a)redhat.com&gt;, "pki-devel" <pki-devel(a)redhat.com&gt; > Sent: Tuesday, March 4, 2014 9:36:52 AM > Subject: Re: [Pki-devel] [PATCH] 0001-First-cut-at-Java-TPS-Buffer-class-and-APDU-class.patch > > Some comments for patch #2: > Done. Sorry, totally spaced building by script. > 1. Build failed due to uncompiled files. See javac command in > base/common/src/CMakeLists.txt. The new namespace should be added into > the javac command (sorry, I forgot to tell you about this). > > javac(pki-certsrv-classes > SOURCES > com/netscape/certsrv/*.java > org/dogtagpki/*.java > > or it probably can be simplified as this: > > javac(pki-certsrv-classes > SOURCES > *.java > > And same thing with the packaging: > > jar(pki-certsrv-jar > FILES > *.class Done, resolved by moving some things around. > 2. Once issue #1 is fixed, there seem to be some dependency issues. The > packages are built in this order: > - base/common (client/common classes) > - base/server/cms (server classes) > - base/server/cmscore (legacy private server classes) > - base/<subsystem: ca,kra,ocsp,tks,tps-tomcat> > A class cannot depend on another class in a lower package. So some > classes may need to be refactored or shuffled around. > Done. > 3. If TPSSession can only be used by the server, it should remain in > base/tps-tomcat. If this is something a client might use, the dependency > should be resolved and the package should be changed into > org.dogtagpki.tps instead of org.dogtagpki.server.tps. > > 4. The TPSServlet probably should remain in base/tps-tomcat because it's > a server class. Decided to take advice to leave and rename as TPSEngine, right now their are only defines, but later will be functionality used by the server. > > 5. The org.dogtagpki.server.tps.engine.TPS class defines many constants. > If the constants are used by clients the class probably should be moved > to org.dogtagpki.tps.TPS. If the constants are used by server only, it > probably should be moved to org.dogtagpki.server.tps.TPSServer, or maybe > merged with TPSSubsystem. But if you'd like to keep it where it is now, > it's probably better to call it TPSEngine. The "TPS" name should be > reserved for namespace (e.g. defining constants, static methods). > Done, sorry some of them simply escaped detection, they are elusive :) > 6. There are some methods that start with uppercase: > - APDU.Set*() > - TPSFormatProcessor.Process() > Generally Java methods start with lowercase. Done. > > 7. An enumeration is used like a class, so it should follow class naming > convention. So the TPSProcessor.TPS_Status probably shouldn't have an > underscore in its name. > Done. Pretty sure I got all the examples of this. > 8. There are some classes with attributes defined at the bottom of the > class. Usually a class is organized as follows: > > public class Something { > // constants > // attributes > // constructors > // methods > }