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-stringbu....
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