Great! Thanks for your patience :) ACK for patch #1-3. -- Endi S. Dewata On 3/4/2014 4:27 PM, John Magne wrote:
Endi thanks for the comments, resolutions below:
----- Original Message -----
From: "Endi Sukma Dewata" <edewata@redhat.com> To: "John Magne" <jmagne@redhat.com>, "pki-devel" <pki-devel@redhat.com> 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 }