LocalVariableCouldBeFinal and MethodArgumentCouldBeFinal use the same detection methods, refactoring

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

pmd/rulesets/optimizations.xml

@@ -10,7 +10,7 @@ These rules deal with different optimizations that generally apply to performanc
   </description>
 
     <rule name="LocalVariableCouldBeFinal"
-          message="Local variable could be declared final"
+	    message="Local variable ''{0}'' could be declared final"
           class="net.sourceforge.pmd.rules.optimization.LocalVariableCouldBeFinal"
           externalInfoUrl="http://pmd.sourceforge.net/rules/optimizations.html#LocalVariableCouldBeFinal">
       <description>

pmd/src/net/sourceforge/pmd/rules/optimization/AbstractOptimizationRule.java

@@ -10,19 +10,8 @@ import java.util.List;
 
 import net.sourceforge.pmd.AbstractRule;
 import net.sourceforge.pmd.Rule;
-import net.sourceforge.pmd.ast.ASTAssignmentOperator;
+import net.sourceforge.pmd.ast.ASTClassOrInterfaceDeclaration;
-import net.sourceforge.pmd.ast.ASTExpression;
+import net.sourceforge.pmd.symboltable.NameOccurrence;
-import net.sourceforge.pmd.ast.ASTLocalVariableDeclaration;
-import net.sourceforge.pmd.ast.ASTMethodDeclaration;
-import net.sourceforge.pmd.ast.ASTName;
-import net.sourceforge.pmd.ast.ASTPostfixExpression;
-import net.sourceforge.pmd.ast.ASTPreDecrementExpression;
-import net.sourceforge.pmd.ast.ASTPreIncrementExpression;
-import net.sourceforge.pmd.ast.ASTPrimaryExpression;
-import net.sourceforge.pmd.ast.ASTPrimaryPrefix;
-import net.sourceforge.pmd.ast.ASTVariableDeclaratorId;
-import net.sourceforge.pmd.ast.Node;
-import net.sourceforge.pmd.ast.SimpleNode;
 
 /**
  * Base class with utility methods for optimization rules
@@ -31,109 +20,21 @@ import net.sourceforge.pmd.ast.SimpleNode;
  */
 public class AbstractOptimizationRule extends AbstractRule implements Rule {
 
-    protected final boolean isVarWritterInMethod(String varName, ASTMethodDeclaration md) {
+    public Object visit(ASTClassOrInterfaceDeclaration node, Object data) {
-        List assignments = md.findChildrenOfType(ASTAssignmentOperator.class);
+        if (node.isInterface()) {
-        return (variableAssigned(varName, assignments) || numericWithPrePost(md, varName));
+            return data;
+        }
+        return super.visit(node, data);
     }
 
-    // TODO - symbol table?
+    protected boolean assigned(List usages) {
-    protected final String getVarName(ASTLocalVariableDeclaration node) {
+        for (Iterator j = usages.iterator(); j.hasNext();) {
-        List l = node.findChildrenOfType(ASTVariableDeclaratorId.class);
+            NameOccurrence occ = (NameOccurrence) j.next();
-        if (!l.isEmpty()) {
+            if (occ.isOnLeftHandSide() || occ.isSelfAssignment()) {
-            ASTVariableDeclaratorId vd = (ASTVariableDeclaratorId) l.get(0);
-            return vd.getImage();
-        }
-        return null;
-    }
-
-    /**
-     * Check constructions like
-     * int i;
-     * ++i;
-     * --i;
-     * i++;
-     * i*=1;
-     * i+=1;
-     */
-    private final boolean numericWithPrePost(ASTMethodDeclaration md, String varName) {
-        // ++i
-        List preinc = md.findChildrenOfType(ASTPreIncrementExpression.class);
-        if (preinc != null && !preinc.isEmpty()) {
-            for (Iterator it = preinc.iterator(); it.hasNext();) {
-                if (isName((Node)it.next(), varName)) {
-                    return true;
-                }
-            }
-        }
-        
-        // --i
-        List predec = md.findChildrenOfType(ASTPreDecrementExpression.class);
-        if (predec != null && !predec.isEmpty()) {
-            for (Iterator it = predec.iterator(); it.hasNext();) {
-                if (isName((Node)it.next(), varName)) {
-                    return true;
-                }
-            }
-        }
-
-        List pf = md.findChildrenOfType(ASTPostfixExpression.class);
-        if (pf != null && !pf.isEmpty()) {
-            for (Iterator it = pf.iterator(); it.hasNext();) {
-                ASTPostfixExpression pe = (ASTPostfixExpression) it.next();
-                if ((pe.hasImageEqualTo("++") || pe.hasImageEqualTo("--"))) {
-                    if (isName(pe, varName)) {
-                        return true;
-                    }
-                }
-            }
-        }
-        return false;
-    }
-
-    private final boolean isName(Node node, String varName) {
-        Node first = node.jjtGetChild(0);
-        Node second = first.jjtGetChild(0);
-        if (second.jjtGetNumChildren() == 0) {
-            return false;
-        }
-        
-        Node third = second.jjtGetChild(0);
-        if (!(third instanceof ASTName)) {
-            if (first instanceof ASTPrimaryExpression) {
-                // deals with extra parenthesis:
-                // "(varName)++" instead of "varName++" for instance
-                if (first.jjtGetNumChildren() != 1) {
-                    return false;
-                }
-                if (second instanceof ASTPrimaryPrefix && second.jjtGetNumChildren() == 1 &&
-                        third instanceof ASTExpression && third.jjtGetNumChildren() == 1) {
-                    return isName(third, varName);
-                }    
-            }
-            return false;
-        }
-        ASTName name = (ASTName) third;
-        return name.hasImageEqualTo(varName);
-    }
-
-    private final boolean variableAssigned(final String varName, final List assignments) {
-        if (assignments == null || assignments.isEmpty()) {
-            return false;
-        }
-        for (Iterator it = assignments.iterator(); it.hasNext();) {
-            final ASTAssignmentOperator a = (ASTAssignmentOperator) it.next();
-            // if node is assigned return true
-            SimpleNode firstChild = (SimpleNode) a.jjtGetParent().jjtGetChild(0);
-            SimpleNode otherChild = (SimpleNode) firstChild.jjtGetChild(0);
-            if (otherChild.jjtGetNumChildren() == 0 || !(otherChild.jjtGetChild(0) instanceof ASTName)) {
-                continue;
-            }
-            ASTName n = (ASTName) otherChild.jjtGetChild(0);
-            if (n.hasImageEqualTo(varName)) {
                 return true;
             }
+            continue;
         }
-
         return false;
     }
 

pmd/src/net/sourceforge/pmd/rules/optimization/AvoidInstantiatingObjectsInLoops.java

@@ -4,7 +4,6 @@
 package net.sourceforge.pmd.rules.optimization;
 
 import net.sourceforge.pmd.ast.ASTAllocationExpression;
-import net.sourceforge.pmd.ast.ASTClassOrInterfaceDeclaration;
 import net.sourceforge.pmd.ast.ASTDoStatement;
 import net.sourceforge.pmd.ast.ASTForStatement;
 import net.sourceforge.pmd.ast.ASTReturnStatement;
@@ -13,14 +12,6 @@ import net.sourceforge.pmd.ast.ASTWhileStatement;
 
 public class AvoidInstantiatingObjectsInLoops extends AbstractOptimizationRule {
 
-    public Object visit(ASTClassOrInterfaceDeclaration node, Object data) {
-        if (node.isInterface()) {
-            return data;
-        }
-        return super.visit(node, data);
-    }
-
-
     public Object visit(ASTAllocationExpression node, Object data) {
         if (insideLoop(node) && fourthParentNotThrow(node) && fourthParentNotReturn(node)) {
             addViolation(data, node);

pmd/src/net/sourceforge/pmd/rules/optimization/LocalVariableCouldBeFinal.java

@@ -3,29 +3,30 @@
  */
 package net.sourceforge.pmd.rules.optimization;
 
-import net.sourceforge.pmd.ast.ASTClassOrInterfaceDeclaration;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+
 import net.sourceforge.pmd.ast.ASTLocalVariableDeclaration;
-import net.sourceforge.pmd.ast.ASTMethodDeclaration;
+import net.sourceforge.pmd.symboltable.Scope;
+import net.sourceforge.pmd.symboltable.VariableNameDeclaration;
 
 public class LocalVariableCouldBeFinal extends AbstractOptimizationRule {
 
-    public Object visit(ASTClassOrInterfaceDeclaration node, Object data) {
-        if (node.isInterface()) {
-            return data;
-        }
-        return super.visit(node, data);
-    }
-
     public Object visit(ASTLocalVariableDeclaration node, Object data) {
         if (node.isFinal()) {
             return data;
         }
-        final String varName = getVarName(node);
+        Scope s = node.getScope();
-
+        Map decls = s.getVariableDeclarations();
-        ASTMethodDeclaration md = (ASTMethodDeclaration) node.getFirstParentOfType(ASTMethodDeclaration.class);
+        for (Iterator i = decls.entrySet().iterator(); i.hasNext();) {
-        if (md != null) {
+            Map.Entry entry = (Map.Entry) i.next();
-            if (!isVarWritterInMethod(varName, md)) {
+            VariableNameDeclaration var = (VariableNameDeclaration) entry.getKey();
-                addViolation(data, node);
+            if (var.getAccessNodeParent() != node) {
+                continue;
+            }
+            if (!assigned((List) entry.getValue())) {
+                addViolation(data, var.getAccessNodeParent(), var.getImage());
             }
         }
         return data;

pmd/src/net/sourceforge/pmd/rules/optimization/MethodArgumentCouldBeFinal.java

@@ -3,25 +3,17 @@
  */
 package net.sourceforge.pmd.rules.optimization;
 
-import net.sourceforge.pmd.ast.ASTClassOrInterfaceDeclaration;
-import net.sourceforge.pmd.ast.ASTFormalParameter;
-import net.sourceforge.pmd.ast.ASTMethodDeclaration;
-import net.sourceforge.pmd.symboltable.NameOccurrence;
-import net.sourceforge.pmd.symboltable.Scope;
-import net.sourceforge.pmd.symboltable.VariableNameDeclaration;
-
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 
-public class MethodArgumentCouldBeFinal extends AbstractOptimizationRule {
+import net.sourceforge.pmd.ast.ASTFormalParameter;
+import net.sourceforge.pmd.ast.ASTMethodDeclaration;
+import net.sourceforge.pmd.ast.AccessNode;
+import net.sourceforge.pmd.symboltable.Scope;
+import net.sourceforge.pmd.symboltable.VariableNameDeclaration;
 
-    public Object visit(ASTClassOrInterfaceDeclaration node, Object data) {
+public class MethodArgumentCouldBeFinal extends AbstractOptimizationRule {
-        if (node.isInterface()) {
-            return data;
-        }
-        return super.visit(node, data);
-    }
 
     public Object visit(ASTMethodDeclaration meth, Object data) {
         if (meth.isNative() || meth.isAbstract()) {
@@ -32,21 +24,12 @@ public class MethodArgumentCouldBeFinal extends AbstractOptimizationRule {
         for (Iterator i = decls.entrySet().iterator(); i.hasNext();) {
             Map.Entry entry = (Map.Entry) i.next();
             VariableNameDeclaration var = (VariableNameDeclaration) entry.getKey();
-            if (!var.getAccessNodeParent().isFinal() && (var.getAccessNodeParent() instanceof ASTFormalParameter) && !assigned((List) entry.getValue())) {
+            AccessNode node = var.getAccessNodeParent();
-                addViolation(data, var.getAccessNodeParent(), var.getImage());
+            if (!node.isFinal() && (node instanceof ASTFormalParameter) && !assigned((List) entry.getValue())) {
+                addViolation(data, node, var.getImage());
             }
         }
         return data;
     }
 
-    private boolean assigned(List usages) {
-        for (Iterator j = usages.iterator(); j.hasNext();) {
-            NameOccurrence occ = (NameOccurrence) j.next();
-            if (occ.isOnLeftHandSide() || occ.isSelfAssignment()) {
-                return true;
-            }
-            continue;
-        }
-        return false;
-    }
 }