Enhancements to UselessStringValueOf thanks to Thomas Leplus

git-svn-id: https://pmd.svn.sourceforge.net/svnroot/pmd/trunk@4857 51baf565-9d33-0410-a72c-fc3788e3496d

pmd/etc/bug_closer.txt

@@ -7,4 +7,4 @@ that contains that fix.
 Thanks,
 
 Tom                  
-The PMD book for $20: http://pmdapplied.com/
+The PMD book - $20 - http://pmdapplied.com/

pmd/etc/changelog.txt

@@ -726,5 +726,5 @@ Added new HTML report format
 June 25 2002 - 0.1:
 Initial release
 
-Using PMD?  Get the book - http://pmdapplied.com/
+The PMD book - $20 - http://pmdapplied.com/
 controversial,basic,strings,design,naming,finalizers,braces,clone,codesize,coupling,imports,javabeans,junit,logging-jakarta-commons,logging-java,migrating,optimizations,strictexception,sunsecure,unusedcode

pmd/regress/test/net/sourceforge/pmd/rules/strings/xml/UselessStringValueOf.xml

@@ -38,6 +38,32 @@ void bar(int i) {
     char low = 'B';
     String s = String.valueOf(c) + low;
 }
+}
+     ]]></code>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+valueOf as first/last expression in concatenation
+     ]]></description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Foo {
+public static String bar25(int n) {
+    return String.valueOf(n) + n + String.valueOf(n); // no - yes
+}
+}
+     ]]></code>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+valueOf as first/last expression in concatenation
+     ]]></description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Foo {
+public static String bar17(int n) {
+    return n + String.valueOf(n) + "n"; // no
+    }
 }
      ]]></code>
     </test-code>

pmd/rulesets/strings.xml

@@ -273,7 +273,7 @@ public class Foo {
           class="net.sourceforge.pmd.rules.strings.UselessStringValueOf"
           externalInfoUrl="http://pmd.sourceforge.net/rules/strings.html#UselessStringValueOf">
       <description>
-Use valueOf() argument directly.
+No need to call String.valueOf to append to a string; just use the valueOf() argument directly.
       </description>
         <priority>3</priority>
       <example>

pmd/src/net/sourceforge/pmd/rules/strings/UselessStringValueOf.java

@@ -7,11 +7,13 @@ import net.sourceforge.pmd.ast.ASTPrimaryExpression;
 import net.sourceforge.pmd.ast.ASTPrimaryPrefix;
 import net.sourceforge.pmd.ast.Node;
 import net.sourceforge.pmd.ast.SimpleJavaNode;
+import net.sourceforge.pmd.symboltable.VariableNameDeclaration;
 
 public class UselessStringValueOf extends AbstractRule {
 
     public Object visit(ASTPrimaryPrefix node, Object data) {
-        if (node.jjtGetNumChildren() == 0 || !node.jjtGetChild(0).getClass().equals(ASTName.class)) {
+        if (node.jjtGetNumChildren() == 0 ||
+            !(node.jjtGetChild(0) instanceof ASTName)) {
             return super.visit(node, data);
         }
 
@@ -19,16 +21,46 @@ public class UselessStringValueOf extends AbstractRule {
 
         if ("String.valueOf".equals(image)) {
             Node parent = node.jjtGetParent();
+            if (parent.jjtGetNumChildren() != 2) {
+                return super.visit(node, data);
+            }
             SimpleJavaNode gp = (SimpleJavaNode) parent.jjtGetParent();
             if (parent instanceof ASTPrimaryExpression &&
                     gp instanceof ASTAdditiveExpression &&
-                    gp.jjtGetChild(0) != parent &&
                     "+".equals(gp.getImage())) {
-                super.addViolation(data, node);
-                return data;
+                boolean ok = false;
+                if (gp.jjtGetChild(0) == parent) {
+                    ok = !isPrimitive(gp.jjtGetChild(1));
+                } else  {
+                    for (int i = 0; !ok && gp.jjtGetChild(i) != parent; i++) {
+                        ok = !isPrimitive(gp.jjtGetChild(i));
+                    }
+                }
+                if (ok) {
+                    super.addViolation(data, node);
+                    return data;
+                }
             }
         }
         return super.visit(node, data);
     }
 
+    private static boolean isPrimitive(Node parent) {
+        boolean result = false;
+        if (parent instanceof ASTPrimaryExpression &&
+            parent.jjtGetNumChildren() == 1 &&
+            parent.jjtGetChild(0) instanceof ASTPrimaryPrefix &&
+            parent.jjtGetChild(0).jjtGetNumChildren() == 1 &&
+            parent.jjtGetChild(0).jjtGetChild(0) instanceof ASTName) {
+            ASTName name = (ASTName) parent.jjtGetChild(0).jjtGetChild(0);
+            if (name.getNameDeclaration() instanceof VariableNameDeclaration) {
+                VariableNameDeclaration nd = (VariableNameDeclaration) name.getNameDeclaration();
+                if (nd.isPrimitiveType()) {
+                    result = true;
+                }
+            }
+        }
+        return result;
+    }
+    
 }

pmd/xdocs/credits.xml

@@ -54,6 +54,7 @@
         </subsection>
         <subsection name="Contributors">
         <ul>
+            <li>Thomas Leplus - Rewrote UselessStringValueOf</li>
             <li>Larry Brigman - Reported symlink bug in CPD</li>
             <li>Harald Rohan - Reported bug in CPD GUI</li>
             <li>classens - Noted missing varargs setting in ASTFormalParameter</li>