[Pki-devel] [PATCH] 0010 code cleanup-com.netscape.cmsutil

[Ade Lee]

CryptoUtil.java:

1. In CryptoUtil.java,  I see a lot of places where the static members
are qualified by their full name.

For example,  content = content.substring(LINE_COUNT); becomes content =
content.substring(CryptoUtil.LINE_COUNT);  Is this some kind of standard
- that local static variables need to be fully qualified?

2. Also in CryptoUtil.java, there are cases where unused variables are
removed incorrectly:  

For example, in public static String reqFormat(String content): 
int beginIndex = CERTREQ_BEGIN_HEADING.length(); becomes ->
CryptoUtil.CERTREQ_BEGIN_HEADING.length();
This happens in multiple places.

3. In public static X509CertInfo createX509CertInfo(), you have the
following change:
 AlgorithmId sigAlgId = new AlgorithmId();  which becomes 
new AlgorithmId();
 and then :
 info.set(X509CertInfo.ALGORITHM_ID,                
          CertificateAlgorithmId(sigAlgId.get(alg)));
becomes: 
info.set(X509CertInfo.ALGORITHM_ID, new
                CertificateAlgorithmId(AlgorithmId.get(alg)));

Is this really right?

The changes in this file as clearly messed up.  Please fix and resubmit.

HttpEofException.java
What is the purpose of the new version UID?

HttpRequest.java
In this file, you also fully specify some static members. Why?

JSSSocketFactory.java
1. You fully qualify a few static members? why?
2. Why does : 
s.enableSSL2Default(false); become SSLSocket.enableSSL2Default(false);
?
3. Why are we removing comments by findCertByNickname?

BasicOCSPResponse.java, CertID.java, GoodInfo.java, KeyHashID.java,
NameID.java, OCSPRequest.java, OCSPResponse.java, OCSPResponseStatus,
Request.java and many others ..
You are qualifying local static members here again.  The question is --
is this an automated and hence consistent thing?  Is this a standard
that needs to be enforced in the code style?

UnknownInfo.java
1. Why are we removing comments here?  The question is one of
consistency -- why are we removing those comments and not the ones in
the decode() function?

CRSPKIMessage.java
1. Good comments removed
2. Its not clear that the variables here ought to be removed.

Utils.java
1. Bad removal of variable in checkHost() -- what is that function doing
in any case?

Any unit tests? 

Ade








 


        
        
On Wed, 2011-11-02 at 17:15 -0400, Adam Young wrote:
> The first of many patches for code
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel