[Pki-devel] [PATCH] 0012 - Fixes for Coverity Issues of type StringBuffer_Concatenation, Wrong_Map_Iterator, No_Equals_Method , Reverse_INull for DogTag 10

[Ade Lee]

On Thu, 2012-06-14 at 19:11 -0500, Endi Sukma Dewata wrote:
> On 6/13/2012 4:31 PM, Abhishek Koneru wrote:
> > Please find attached PATCH 12 - with fixes for issues in Coverity of
> > types StringBuffer_Concatenation, Wrong_Map_Iterator, No_Equals_Method ,
> > Reverse_INull for review.
> 
> Some issues:
> 
> 1. There's a formatting issue in CMSCRLExtensions.java:586.
> 
> 2. In CRLIssuingPoint.java:2712-2737 the String concatenations are 
> replaced with StringBuffer. This is fine and not a big deal but it can 
> be done more efficiently. See http://kaioa.com/node/59 and 
> http://javarevisited.blogspot.com/2011/07/string-vs-stringbuffer-vs-stringbuilder.html.
> 
> In this code the only thread using the StringBuffer is the current 
> thread itself, so there's no concurrency issue. So the StringBuffer can 
> be replaced with StringBuilder.
> 
> Every time you do a "+" operation on Strings, the JVM will create a 
> StringBuilder, append the Strings, then generate a new String that 
> contains the concatenated text. So instead of this:
> 
>    splitTimes.append(
>        "," + Long.toString(deltaTime) + "," + Long.toString(crlTime)
>        + "," + Long.toString(totalTime) + ")");
> 
> you can reuse the same StringBuilder object like this:
> 
>    splitTimes.append(",").append(deltaTime).append(",").append(crlTime)
>        .append(",").append(totalTime).append(")");
> 

While I appreciate that the above formulation is more efficient, there
is a drawback in terms of readability.  This following formulation may
be a little less efficient but is much more readable:

splitTimes.append("," + deltaTime + "," + crlTime + "," + totalTime + ")");

I suggest that we not sacrifice readability for efficiency until we have
profiling data in hand. I suspect that this little inefficiency will be
the very least of our performance issues. 

> Notice the Long.toString() is not necessary because there are separate 
> append() methods for each primitive type. Also with autoboxing the 
> primitive types will be converted automatically to objects, so the code 
> in lines 2722, 2729-2738 can be simplified like this:
> 
>    splitTimes.append(mSplits[i]);
> 
>    new Object[] {
>        getId(),
>        getCRLNumber(),
>        getLastUpdate(),
>        getNextUpdate(),
>        mCRLSize,
>        totalTime,
>        crlTime,
>        "" + deltaTime + splitTimes
>    }
> 
> 3. In CRLIssuingPoint.java:3098-3105,3151 whenever you remove the { } 
> surrounding a code block you should fix the indentation.
> 
> 4. In NameValuePairs.java:47 you can use StringBuilder too. In line 51 
> it's better to use separate append() for each String component.
> 
> 5. In SimpleProperties.java you should use Eclipse to generate 
> hashCode() too, it goes together with equals().
> 
> 6. In Auditor.java the StringBuilder can be created once at line 99, 
> then the subsequent code will append each String component separately.
> 
> 7. In ACL.java:153 you can use StringBuilder. In lines 159 and 163 it's 
> not necessary to call toString() explicitly.
> 
> 8. In ExtDataHashtable.java:59 I think you can actually remove the 
> putAll() because it's identical to the one in the super class.
> 
> 9. In RequestTest.java you should add hashCode() as well.
> 
> 10. Same thing in CMCResponse.java:125-130, use StringBuilder, separate 
> appends, no explicit toString().
> 
> 11. Same thing in PrettyPrintCert.java:89,200,221, use StringBuilder, 
> separate appends, no explicit toString().
> 
> 12. Add hashCode() to the following files:
>   - CMSProperties.java
>   - PKCS10Attributes.java
>   - DSAPrivateKey.java
>   - DSAPublicKey.java
>   - RSAPublicKey.java
>   - AlgIdDSA.java
>   - CRLExtensions.java
>   - CertificateExtensions.java
>   - Extensions.java
>