hiifong/pmd
1
0
Fork 0
forked from phoedos/pmd
Code Pull Requests Activity

Fixed UseCollectionIsEmpty rule to only fire on Collection types. Made the code abstract since it's very similar to InefficientEmptyStringCheck.

Browse Source 
The code also only found lst.size() == 0, and not 0 == lst.size(), that's fixed too.


git-svn-id: https://pmd.svn.sourceforge.net/svnroot/pmd/trunk@4748 51baf565-9d33-0410-a72c-fc3788e3496d
This commit is contained in:
Allan Caplan
2006-10-26 17:28:59 +00:00
parent 6c5338fdaf
commit afb64b04a6
4 changed files with 126 additions and 107 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
pmd/regress/test/net/sourceforge/pmd/rules/design/UseCollectionIsEmptyTest.java
View File

@ -33,6 +33,7 @@ public class UseCollectionIsEmptyTest extends SimpleAggregatorTst{
new TestDescriptor(TEST4, "ok, !isEmpty", 0, rule),
new TestDescriptor(TEST5, "fail, != 0", 1, rule),
new TestDescriptor(TEST6, "ok, !isEmpty", 0, rule),
new TestDescriptor(TEST7, "fail, 0 ==", 1, rule),
});
}
@ -96,5 +97,14 @@ public class UseCollectionIsEmptyTest extends SimpleAggregatorTst{
" }" + PMD.EOL +
"}";
}
private static final String TEST7 =
"public class Foo {" + PMD.EOL +
" public static boolean bar(List lst) {" + PMD.EOL +
" if(0 == lst.size()){" + PMD.EOL +
" return true;" + PMD.EOL +
" }" + PMD.EOL +
" return false;" + PMD.EOL +
" }" + PMD.EOL +
"}";}

76
pmd/src/net/sourceforge/pmd/rules/AbstractInefficientZeroCheck.java Normal file
View File

@ -0,0 +1,76 @@
package net.sourceforge.pmd.rules;
import net.sourceforge.pmd.AbstractRule;
import net.sourceforge.pmd.ast.ASTEqualityExpression;
import net.sourceforge.pmd.ast.ASTLiteral;
import net.sourceforge.pmd.ast.ASTPrimitiveType;
import net.sourceforge.pmd.ast.ASTVariableDeclaratorId;
import net.sourceforge.pmd.ast.SimpleNode;
import net.sourceforge.pmd.symboltable.NameOccurrence;
import java.util.Iterator;
import java.util.List;
/**
* This is an abstract rule for patterns which compare a method invocation to 0.
* It could be further abstracted to find code that compares something to
* another definable pattern
*
* @author acaplan
*/
public abstract class AbstractInefficientZeroCheck extends AbstractRule {
public abstract boolean appliesToClassName(String name);
public abstract boolean isTargetMethod(NameOccurrence occ);
public Object visit(ASTVariableDeclaratorId node, Object data) {
SimpleNode nameNode = node.getTypeNameNode();
if (nameNode instanceof ASTPrimitiveType) {
return data;
}
if (!appliesToClassName(node.getNameDeclaration().getTypeImage())) {
return data;
}
List declars = node.getUsages();
for (Iterator i = declars.iterator(); i.hasNext();) {
NameOccurrence occ = (NameOccurrence) i.next();
if (!isTargetMethod(occ)) {
continue;
}
ASTEqualityExpression equality = (ASTEqualityExpression) occ.getLocation().getFirstParentOfType(
ASTEqualityExpression.class);
if (equality != null && isCompareZero(equality)) {
addViolation(data, occ.getLocation());
}
}
return data;
}
/**
* We only need to report if this is comparing against 0
*
* @param equality
* @return true if this is comparing to 0 else false
*/
private boolean isCompareZero(ASTEqualityExpression equality) {
return (checkComparison(equality, 0) || checkComparison(equality, 1));
}
/**
* Checks if the equality expression passed in is of comparing against the
* value passed in as i
*
* @param equality
* @param i
* The ordinal in the equality expression to check
* @return true if the value in position i is 0, else false
*/
private boolean checkComparison(ASTEqualityExpression equality, int i) {
SimpleNode target = (SimpleNode) equality.jjtGetChild(i).jjtGetChild(0).jjtGetChild(0);
return (target instanceof ASTLiteral && "0".equals(target.getImage()));
}
}

60
pmd/src/net/sourceforge/pmd/rules/design/UseCollectionIsEmpty.java
View File

@ -3,13 +3,10 @@
*/
package net.sourceforge.pmd.rules.design;
import net.sourceforge.pmd.AbstractRule;
import net.sourceforge.pmd.ast.ASTEqualityExpression;
import net.sourceforge.pmd.ast.ASTLiteral;
import net.sourceforge.pmd.ast.ASTName;
import net.sourceforge.pmd.ast.ASTPrimaryPrefix;
import net.sourceforge.pmd.ast.ASTPrimarySuffix;
import net.sourceforge.pmd.ast.SimpleJavaNode;
import net.sourceforge.pmd.ast.SimpleNode;
import net.sourceforge.pmd.rules.AbstractInefficientZeroCheck;
import net.sourceforge.pmd.symboltable.NameOccurrence;
import net.sourceforge.pmd.util.CollectionUtil;
/**
* Detect structures like "foo.size() == 0" and suggest replacing them with
@ -17,36 +14,25 @@ import net.sourceforge.pmd.ast.SimpleJavaNode;
*
* @author Jason Bennett
*/
public class UseCollectionIsEmpty extends AbstractRule {
public Object visit(ASTEqualityExpression node, Object data) {
SimpleJavaNode leftSide = (SimpleJavaNode) node.jjtGetChild(0);
SimpleJavaNode rightSide = (SimpleJavaNode) node.jjtGetChild(1);
ASTPrimaryPrefix leftPrefix = (ASTPrimaryPrefix) leftSide.getFirstChildOfType(ASTPrimaryPrefix.class);
ASTPrimarySuffix leftSuffix = (ASTPrimarySuffix) leftSide.getFirstChildOfType(ASTPrimarySuffix.class);
ASTName leftName = null;
if (leftPrefix != null) {
leftName = (ASTName) leftPrefix.getFirstChildOfType(ASTName.class);
public class UseCollectionIsEmpty extends AbstractInefficientZeroCheck {
public boolean appliesToClassName(String name){
return CollectionUtil.isCollectionType(name, true);
}
/**
* Determine if we're dealing with .size method
*
* @param occ
* The name occurance
* @return true if it's .length, else false
*/
public boolean isTargetMethod(NameOccurrence occ) {
if (occ.getNameForWhichThisIsAQualifier() != null) {
if (((SimpleNode) occ.getLocation()).getImage().endsWith(".size")) {
return true;
}
}
if (leftName == null || leftSuffix == null) {
return data;
}
ASTPrimaryPrefix rightPrefix = (ASTPrimaryPrefix) rightSide.getFirstChildOfType(ASTPrimaryPrefix.class);
ASTLiteral rightLiteral = null;
if (rightPrefix != null) {
rightLiteral = (ASTLiteral) rightPrefix.getFirstChildOfType(ASTLiteral.class);
}
if (rightLiteral != null && leftSuffix.isArguments() && leftSuffix.getArgumentCount() == 0
&& leftName.getImage().endsWith(".size") && rightLiteral.hasImageEqualTo("0")) {
addViolation(data, node);
}
return data;
return false;
}
}

85
pmd/src/net/sourceforge/pmd/rules/strings/InefficientEmptyStringCheck.java
View File

@ -1,103 +1,50 @@
package net.sourceforge.pmd.rules.strings;
import net.sourceforge.pmd.AbstractRule;
import net.sourceforge.pmd.ast.ASTEqualityExpression;
import net.sourceforge.pmd.ast.ASTLiteral;
import net.sourceforge.pmd.ast.ASTPrimitiveType;
import net.sourceforge.pmd.ast.ASTVariableDeclaratorId;
import net.sourceforge.pmd.ast.Node;
import net.sourceforge.pmd.ast.SimpleNode;
import net.sourceforge.pmd.rules.AbstractInefficientZeroCheck;
import net.sourceforge.pmd.symboltable.NameOccurrence;
import java.util.Iterator;
import java.util.List;
/**
* This rule finds code which inefficiently determines empty strings. This code
* <p/>
*
* <pre>
* if(str.trim().length()==0){....
* if(str.trim().length()==0){....
* </pre>
* <p/>
* is quite inefficient as trim() causes a new String to be created. Smarter
* code to check for an empty string would be:
* <p/>
*
* <p/> is quite inefficient as trim() causes a new String to be created.
* Smarter code to check for an empty string would be: <p/>
*
* <pre>
* Character.isWhitespace(str.charAt(i));
* </pre>
*
*
* @author acaplan
*/
public class InefficientEmptyStringCheck extends AbstractRule {
public Object visit(ASTVariableDeclaratorId node, Object data) {
SimpleNode nameNode = node.getTypeNameNode();
if (nameNode instanceof ASTPrimitiveType) {
return data;
}
if (!"String".equals(node.getNameDeclaration().getTypeImage())) {
return data;
}
List declars = node.getUsages();
for (Iterator i = declars.iterator(); i.hasNext();) {
NameOccurrence occ = (NameOccurrence) i.next();
if (!isStringLength(occ)) {
continue;
}
ASTEqualityExpression equality = (ASTEqualityExpression) occ
.getLocation().getFirstParentOfType(ASTEqualityExpression.class);
if (equality != null && isCompareZero(equality)) {
addViolation(data, occ.getLocation());
}
}
return data;
}
/**
* We only need to report if this is comparing against 0
*
* @param equality
* @return true if this is comparing to 0 else false
*/
private boolean isCompareZero(ASTEqualityExpression equality) {
return (checkComparison(equality, 0) || checkComparison(equality, 1));
}
public class InefficientEmptyStringCheck extends AbstractInefficientZeroCheck {
/**
* Determine if we're dealing with String.length method
*
* @param occ The name occurance
*
* @param occ
* The name occurance
* @return true if it's String.length, else false
*/
private boolean isStringLength(NameOccurrence occ) {
public boolean isTargetMethod(NameOccurrence occ) {
if (occ.getNameForWhichThisIsAQualifier() != null
&& occ.getNameForWhichThisIsAQualifier().getImage().indexOf("trim") != -1) {
Node pExpression = occ.getLocation().jjtGetParent().jjtGetParent();
if (pExpression.jjtGetNumChildren() >= 3
&& "length"
.equals(((SimpleNode) pExpression.jjtGetChild(2))
.getImage())) {
&& "length".equals(((SimpleNode) pExpression.jjtGetChild(2)).getImage())) {
return true;
}
}
return false;
}
/**
* Checks if the equality expression passed in is of comparing against the
* value passed in as i
*
* @param equality
* @param i The ordinal in the equality expression to check
* @return true if the value in position i is 0, else false
*/
private boolean checkComparison(ASTEqualityExpression equality, int i) {
return (equality.jjtGetChild(i).jjtGetChild(0).jjtGetChild(0) instanceof ASTLiteral && "0"
.equals(((SimpleNode) equality.jjtGetChild(i).jjtGetChild(0)
.jjtGetChild(0)).getImage()));
public boolean appliesToClassName(String name) {
return "String".equals(name);
}
}