Re: [Pki-devel] [PATCH] 0001-KRA-changes-for-archiving-and-recovering-symmetric-keys.patch

On 2/8/2012 1:36 PM, John Magne wrote: These are just some minor issues/nitpicking, but I hope it will lead to cleaner code in the long run. Some of these might have been discussed already. 1. The throw statement in KeyDAO.java:124 for null params can be moved right after the params assignment in line 111. In general I think we should check null right after assignment and before doing anything else. 2. The keyData null checking In KeyDAO.java:160,168 is unnecessary because in all possible code execution the keyData always gets a new instance (line 128 and 159). The 'new' operation never returns a null, if it fails for any reason it will throw an exception so the check will never be executed. 3. The null initialization for params in KeyRequestDAO.java:230 is unnecessary because the code guarantees that params will be initialized with a value (line 231) before it's read for the first time (line 233). In general if we forgot to initialize a variable the code will fail to build, so we don't need to worry about removing unnecessary null assignments. 4. Formatting issue, some expressions/params between parentheses have unbalanced spaces: if ( expression) { method( params); I'm not sure if we have a coding standard for this, but at least it should be consistent within a single statement, either use spaces in both positions or not at all: if ( expression ) { if (expression) { 5. Similar to #3, the null initialization for IV in DRMTest.java:133 is unnecessary because the following try-catch clause guarantees that it will be initialized regardless of exceptions. Similar reason for IV_server in line 134. 6. According to our coding standard the variable names should begin with lower case. So in #5 it should be iv and iv_server. 7. The toString() in DRMTest.java:139 (and some other places) is unnecessary. In general any string concatenation will automatically call the toString() of the object. 8. The extra parentheses in DRMTest.java:562 is unnecessary because the compiler can distinguish the constructor from the method. The following code will work just fine: new PKIArchiveOptions.Template().decode(inStream); 9. In ArchiveOptions class (and some other new classes) the member variables are prefixed with 'm': private String mSymmAlgOID = null; private byte mSymmAlgParams[] = null; private byte mEncSymmKey[] = null; private byte mEncValue[] = null; I think we discussed previously that the prefix is unnecessary, so no need to add it in new code. The null initialization is also unnecessary because all member variables are assigned to null by default during instantiation. 10. Array constants like in EncryptionUnit.java:652 can be defined like this for simplicity: SymmetricKey.Usage usages[] = { SymmetricKey.Usage.WRAP, SymmetricKey.Usage.UNWRAP }; 11. In SecurityDataRecoveryService.java the assignment in line 168 can be moved into the catch clause above it because iv will only be null if there's an exception. The nextBytes() will not make iv null because method parameters are passed by value. The clone() is not necessary either, iv can just point to the iv_default. The rnd can also be moved inside the try block to minimize variable scope. iv = new byte[8]; try { Random rnd = new Random(); rnd.nextBytes(iv); } catch (Exception e) { iv = iv_default; } -- Endi S. Dewata