New PrematureDeclaration rule checks for variable declared at suboptimal positions within methods. Tests seem ok but if you guys could also run it over your preferred code samples that would be great.

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

pmd/regress/test/net/sourceforge/pmd/lang/java/rule/optimizations/OptimizationsRulesTest.java

@@ -21,6 +21,7 @@ public class OptimizationsRulesTest extends SimpleAggregatorTst {
         addRule(RULESET, "UseArrayListInsteadOfVector");
         addRule(RULESET, "UseArraysAsList");
         addRule(RULESET, "UseStringBufferForStringAppends");
+        addRule(RULESET, "PrematureDeclaration");
     }
 
     public static junit.framework.Test suite() {

pmd/regress/test/net/sourceforge/pmd/lang/java/rule/optimizations/xml/PrematureDeclaration.xml

@@ -0,0 +1,48 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<test-data>
+    <test-code>
+        <description><![CDATA[
+premature declaration before unrelated test
+     ]]></description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Bar {
+	public int lengthSumOf(String[] strings) {
+		
+		int sum = 0;	// wasted cycles if strings have problems
+		
+		if (strings == null || strings.length == 0) return 0;
+		
+		for (int i=0; i<strings.length; i++) {
+			sum += strings[i].length();
+		}
+		
+		return sum;
+	}
+}
+     ]]></code>
+    </test-code>
+    <test-code>
+        <description><![CDATA[
+optimal var declaration position
+     ]]></description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Bar {
+	public int lengthSumOf(String[] strings) {
+				
+		if (strings == null || strings.length == 0) return 0;
+				
+		int sum = 0;	// optimal placement
+		
+		for (int i=0; i<strings.length; i++) {
+			sum += strings[i].length();
+		}
+		
+		return sum;
+	}
+}
+     ]]></code>
+    </test-code>
+
+</test-data>

pmd/rulesets/java/optimizations.xml

@@ -394,6 +394,34 @@ public class C {
               </example>
             </rule>
 
+	<rule name="PrematureDeclaration"
+			language="java"
+			since="5.0"
+			message="Avoid declaring a variable if it is unreferenced before a possible exit point."
+			class="net.sourceforge.pmd.lang.java.rule.optimizations.PrematureDeclarationRule"
+			externalInfoUrl="http://pmd.sourceforge.net/rules/java/optimizations.html#PrematureDeclaration">
+		<description>
+Checks for variables that are defined before they might be used. A reference is deemed to be premature if it is created right before a block of code that doesn'tuse it that also has the ability to return or throw an exception.
+		</description>
+              <priority>3</priority>
+              <example>
+              <![CDATA[
+public int getLength(String[] strings) {
+  
+  int length = 0;	// declared prematurely
+
+  if (strings == null || strings.length == 0) return 0;
+  
+  for (String str : strings) {
+    length += str.length();
+    }
+
+  return length;
+}
+              ]]>
+              </example>
+            </rule>
+
     <!--
     other optimization should be like avoiding
     "" + int

pmd/src/net/sourceforge/pmd/lang/java/rule/optimizations/PrematureDeclarationRule.java

@@ -0,0 +1,198 @@
+package net.sourceforge.pmd.lang.java.rule.optimizations;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import net.sourceforge.pmd.lang.ast.Node;
+import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement;
+import net.sourceforge.pmd.lang.java.ast.ASTForInit;
+import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTName;
+import net.sourceforge.pmd.lang.java.ast.ASTReturnStatement;
+import net.sourceforge.pmd.lang.java.ast.ASTThrowStatement;
+import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclarator;
+import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
+import net.sourceforge.pmd.lang.java.ast.AbstractJavaNode;
+import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
+
+/**
+ * Checks for variables that are defined before they are really needed. A reference is
+ * deemed to be premature if it is created ahead of a block of code that doesn't
+ * use it that also has the ability to return or throw an exception.
+ * 
+ * @author Brian Remedios
+ * @version $Revision: 1.0 $
+ */
+public class PrematureDeclarationRule extends AbstractJavaRule {
+
+	public PrematureDeclarationRule() { }
+
+    /**
+     *
+     * @param node ASTLocalVariableDeclaration
+     * @param data Object
+     * @return Object
+     * @see net.sourceforge.pmd.ast.JavaParserVisitor#visit(ASTLocalVariableDeclaration, Object)
+     */
+    public Object visit(ASTLocalVariableDeclaration node, Object data) {
+    	
+    	// is it part of a for-loop declaration?
+    	if (node.jjtGetParent().getClass().equals(ASTForInit.class)) {;
+    	   return visit((AbstractJavaNode) node, data);	// yes, those don't count
+    	}
+    	
+    	String varName = varNameIn(node);
+    	
+    	AbstractJavaNode grandparent = (AbstractJavaNode)node.jjtGetParent().jjtGetParent();
+   	
+       	List<Node> nextBlocks = blocksAfter(grandparent, node);
+       	
+       	ASTBlockStatement statement;
+       	
+       	for (Node block : nextBlocks) {
+       		
+       		statement = (ASTBlockStatement)block;
+       		
+       		if (hasReferencesIn(statement, varName)) break;
+       		
+       		if (hasExit(statement)) {
+       			addViolation(data, node, varName);
+       			break;
+       		}
+       	}       	
+       	
+        return visit((AbstractJavaNode) node, data);
+    }
+    
+    /**
+     * Return whether a class of the specified type exists between the node argument
+     * and the topParent argument.
+     * 
+     * @param node Node
+     * @param intermediateParentClass Class
+     * @param topParent Node
+     * @return boolean
+     */
+    public static boolean hasAsParentBetween(Node node, Class<?> intermediateParentClass, Node topParent) {
+    	
+    	Node currentParent = node.jjtGetParent();
+    	
+    	while (currentParent != topParent) {
+    		currentParent = currentParent.jjtGetParent();
+    		if (currentParent.getClass().equals(intermediateParentClass)) return true;
+    	}
+    	return false;
+    }
+    
+    /**
+     * Returns whether the block contains a return call or throws an exception.
+     * Exclude blocks that have these things as part of an inner class.
+     * 
+     * @param block ASTBlockStatement
+     * @return boolean
+     */
+    private boolean hasExit(ASTBlockStatement block) {
+    	
+    	List exitBlocks = block.findDescendantsOfType(ASTReturnStatement.class);
+    	exitBlocks.addAll(block.findDescendantsOfType(ASTThrowStatement.class));
+    	
+    	if (exitBlocks.isEmpty()) return false;
+    	
+    	// now check to see if the ones we have are part of a method on a declared inner class
+    	for (int i=0; i<exitBlocks.size(); i++) {
+    		Node exitNode = (Node)exitBlocks.get(i);
+    		if (hasAsParentBetween(exitNode, ASTMethodDeclaration.class, block)) continue;
+    		return true;
+    	}
+    	
+    	return false;
+    }
+    
+    /**
+     * Returns whether the variable is mentioned within the statement block
+     * or not.
+     * 
+     * @param block ASTBlockStatement
+     * @param varName String
+     * @return boolean
+     */
+    private static boolean hasReferencesIn(ASTBlockStatement block, String varName) {
+    	
+    	List<ASTName> names = block.findDescendantsOfType(ASTName.class);
+    	
+    	for (ASTName name : names) {    		
+    		if (isReference(varName, name.getImage())) return true;
+    	}
+    	return false;
+    }
+    
+    /**
+     * Return whether the shortName is part of the compound name
+     * by itself or as a method call receiver.
+     * 
+     * @param shortName String
+     * @param compoundName String
+     * @return boolean
+     */
+    private static boolean isReference(String shortName, String compoundName) {
+    	
+    	int dotPos = compoundName.indexOf('.');
+
+    	return dotPos < 0 ?
+    		shortName.equals(compoundName) :
+    		shortName.endsWith(compoundName.substring(0, dotPos));
+    }
+    
+    /**
+     * Return the name of the variable we just assigned something to.
+     * 
+     * @param node ASTLocalVariableDeclaration
+     * @return String
+     */
+    private static String varNameIn(ASTLocalVariableDeclaration node) {
+    	
+    	ASTVariableDeclarator declarator = (ASTVariableDeclarator)node.jjtGetChild(1);
+    	return ((ASTVariableDeclaratorId) declarator.jjtGetChild(0)).getImage();
+    } 
+    
+    /**
+     * Returns the index of the node block in relation to its siblings.
+     * 
+     * @param block SimpleJavaNode
+     * @param node Node
+     * @return int
+     */
+    private static int indexOf(AbstractJavaNode block, Node node) {
+    
+    	int count = block.jjtGetNumChildren();
+    	
+    	for (int i=0; i<count; i++) {
+    		if (node == block.jjtGetChild(i)) return i;
+    	}
+    	
+    	return -1;
+    }
+    
+    /**
+     * Returns all the blocks found right after the node supplied within
+     * the its current scope.
+     * 
+     * @param block SimpleJavaNode
+     * @param node SimpleNode
+     * @return List
+     */
+    private static List<Node> blocksAfter(AbstractJavaNode block, AbstractJavaNode node) {
+    	
+    	int count = block.jjtGetNumChildren();
+    	int start = indexOf(block, node.jjtGetParent()) + 1;
+    	    	
+    	List<Node> nextBlocks = new ArrayList<Node>(count);
+    	
+    	for (int i=start; i<count; i++) {
+    		nextBlocks.add(block.jjtGetChild(i));
+    	}
+    	
+    	return nextBlocks;
+    }
+}