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

[Endi Sukma Dewata]

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 at redhat.com>
>> To: "John Magne" <jmagne at redhat.com>, "pki-devel" <pki-devel at 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
>>     }