[Pki-devel] [PATCH] 27-2 Fixes for review comments on [PATCH] 27

[Endi Sukma Dewata]

On 8/2/2012 6:23 PM, Abhishek Koneru wrote:
> Please review the attached patch with fixes for comments given on Patch
> 27.

Some more comments:

>> 11. The CertSearchData.valueOf() should take a more generic input, e.g.
>> Reader, so it can be used to read other inputs, not just files.

I think we should use a Reader class instead of InputStream when 
handling text data, the caller can use FileReader instead of 
FileInputStream.

12. In CertFindCLI.java:111 let's add a space after the dot to separate 
the exception message.

13. In CertFindCLI.java:162,265,267 I think we should use "uid" instead 
of "id" as the parameter name to make it clearer.

14. In CertFindCLI.java:162 we should add an argument name "user id".

15. In CertFindCLI.java:187,190,296,298,300,302 the current 
"revokedFrom" and "revokedTill" might be interpreted differently. Let's 
use the same parameter names as the field names in CertSearchData, in 
this case "revokedOnFrom" and "revokedOnTo" (we can revise it again 
later). And the following parameter descriptions might be better.
  * revokedOnFrom: Revoked on or after this date
  * revokedOnTo: Revoked on or before this date

16. In CertFindCLI.java:183 the "revokedBy" takes a "user id" as an 
argument instead of "serial number".

17. Similarly, in CertFindCLI.java:201 the "issuedBy" takes a "user id".

18. In CertFindCLI.java:149,152 let's use "serial number" as the command 
argument instead of just "number".

19. In CertFindCLI.java:210-224 the certType* parameters take either on 
or off value. So the arg names should be "on|off".

You can also go to the Agent's Certificate Manger -> Search for 
Certificates to see how the parameters are used in the UI.

One more thing, I think we don't need those ...InUse fields in the 
CertSearchData because they can be implied from the parameters we 
specify. They can be removed in a separate ticket.

-- 
Endi S. Dewata