From 6bf8138aaf58784581df62fde5c76a9ec58eabdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Wed, 22 Mar 2017 10:30:50 +0100 Subject: [PATCH 1/4] Added violation case for InefficientEmptyStringCheckRule String.trim().isEmpty() now triggers a violation. Fixed checkstyle --- .../InefficientEmptyStringCheckRule.java | 54 ++++++++++++------- .../InefficientEmptyStringCheckTest.java | 21 ++++++++ .../xml/InefficientEmptyStringCheck.xml | 48 +++++++++++++++++ 3 files changed, 103 insertions(+), 20 deletions(-) create mode 100644 pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/strings/InefficientEmptyStringCheckTest.java diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/strings/InefficientEmptyStringCheckRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/strings/InefficientEmptyStringCheckRule.java index 97dc97ee60..cbd6f7b86f 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/strings/InefficientEmptyStringCheckRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/strings/InefficientEmptyStringCheckRule.java @@ -4,49 +4,63 @@ package net.sourceforge.pmd.lang.java.rule.strings; import net.sourceforge.pmd.lang.ast.Node; +import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression; import net.sourceforge.pmd.lang.java.rule.AbstractInefficientZeroCheck; import net.sourceforge.pmd.lang.java.symboltable.JavaNameOccurrence; /** * This rule finds code which inefficiently determines empty strings. This code - *

- * + * * 

  *         if(str.trim().length()==0){....
  *
- * - *

is quite inefficient as trim() causes a new String to be created. - * Smarter code to check for an empty string would be:

- * + * + *

+ * is quite inefficient as trim() causes a new String to be created. Smarter + * code to check for an empty string would be: + *

+ * * 
  * Character.isWhitespace(str.charAt(i));
  *
- * + * * @author acaplan */ public class InefficientEmptyStringCheckRule extends AbstractInefficientZeroCheck { - - /** - * Determine if we're dealing with String.length method - * - * @param occ - * The name occurrence - * @return true if it's String.length, else false - */ + + @Override public boolean isTargetMethod(JavaNameOccurrence occ) { if (occ.getNameForWhichThisIsAQualifier() != null && occ.getNameForWhichThisIsAQualifier().getImage().indexOf("trim") != -1) { Node pExpression = occ.getLocation().jjtGetParent().jjtGetParent(); - if (pExpression.jjtGetNumChildren() >= 3 - && "length".equals(pExpression.jjtGetChild(2).getImage())) { + if (pExpression.jjtGetNumChildren() > 2 && "length".equals(pExpression.jjtGetChild(2).getImage())) { return true; } } return false; } - + + @Override public boolean appliesToClassName(String name) { return "String".equals(name); } - -} \ No newline at end of file + + @Override + public Object visit(ASTPrimaryExpression node, Object data) { + + if (node.jjtGetNumChildren() > 3) { + // Check last suffix + if (!"isEmpty".equals(node.jjtGetChild(node.jjtGetNumChildren() - 2).getImage())) { + return data; + } + + Node prevCall = node.jjtGetChild(node.jjtGetNumChildren() - 4); + String target = prevCall.jjtGetNumChildren() > 0 ? prevCall.jjtGetChild(0).getImage() : prevCall.getImage(); + if (target != null && target.indexOf("trim") != -1) { + addViolation(data, node); + } + } + return data; + } + +} diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/strings/InefficientEmptyStringCheckTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/strings/InefficientEmptyStringCheckTest.java new file mode 100644 index 0000000000..8faf347a70 --- /dev/null +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/strings/InefficientEmptyStringCheckTest.java @@ -0,0 +1,21 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.rule.strings; + +import net.sourceforge.pmd.testframework.SimpleAggregatorTst; + +/** + * @author Clément Fournier (clement.fournier@insa-rennes.fr) + * + */ +public class InefficientEmptyStringCheckTest extends SimpleAggregatorTst { + + private static final String RULESET = "java-strings"; + + @Override + public void setUp() { + addRule(RULESET, "InefficientEmptyStringCheck"); + } +} diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/strings/xml/InefficientEmptyStringCheck.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/strings/xml/InefficientEmptyStringCheck.xml index e9605f5469..02b6347c40 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/strings/xml/InefficientEmptyStringCheck.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/strings/xml/InefficientEmptyStringCheck.xml @@ -132,4 +132,52 @@ public class Foo { } ]]> + + + 1 + + + + + + 2 + + + + + + 2 + + + From 3454c6eb4e4663285394af84768fd5287984782f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Wed, 22 Mar 2017 17:29:37 +0100 Subject: [PATCH 2/4] Removed test class --- .../InefficientEmptyStringCheckTest.java | 21 ------------------- 1 file changed, 21 deletions(-) delete mode 100644 pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/strings/InefficientEmptyStringCheckTest.java diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/strings/InefficientEmptyStringCheckTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/strings/InefficientEmptyStringCheckTest.java deleted file mode 100644 index 8faf347a70..0000000000 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/strings/InefficientEmptyStringCheckTest.java +++ /dev/null @@ -1,21 +0,0 @@ -/** - * BSD-style license; for more info see http://pmd.sourceforge.net/license.html - */ - -package net.sourceforge.pmd.lang.java.rule.strings; - -import net.sourceforge.pmd.testframework.SimpleAggregatorTst; - -/** - * @author Clément Fournier (clement.fournier@insa-rennes.fr) - * - */ -public class InefficientEmptyStringCheckTest extends SimpleAggregatorTst { - - private static final String RULESET = "java-strings"; - - @Override - public void setUp() { - addRule(RULESET, "InefficientEmptyStringCheck"); - } -} From c62ad45cf62062758aaf65fb56d2c360ba458222 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Wed, 22 Mar 2017 18:50:30 +0100 Subject: [PATCH 3/4] Specialized condition --- .../strings/InefficientEmptyStringCheckRule.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/strings/InefficientEmptyStringCheckRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/strings/InefficientEmptyStringCheckRule.java index cbd6f7b86f..b465a8713b 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/strings/InefficientEmptyStringCheckRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/strings/InefficientEmptyStringCheckRule.java @@ -27,7 +27,7 @@ import net.sourceforge.pmd.lang.java.symboltable.JavaNameOccurrence; * @author acaplan */ public class InefficientEmptyStringCheckRule extends AbstractInefficientZeroCheck { - + @Override public boolean isTargetMethod(JavaNameOccurrence occ) { if (occ.getNameForWhichThisIsAQualifier() != null @@ -39,28 +39,28 @@ public class InefficientEmptyStringCheckRule extends AbstractInefficientZeroChec } return false; } - + @Override public boolean appliesToClassName(String name) { return "String".equals(name); } - + @Override public Object visit(ASTPrimaryExpression node, Object data) { - + if (node.jjtGetNumChildren() > 3) { // Check last suffix if (!"isEmpty".equals(node.jjtGetChild(node.jjtGetNumChildren() - 2).getImage())) { return data; } - + Node prevCall = node.jjtGetChild(node.jjtGetNumChildren() - 4); String target = prevCall.jjtGetNumChildren() > 0 ? prevCall.jjtGetChild(0).getImage() : prevCall.getImage(); - if (target != null && target.indexOf("trim") != -1) { + if (target != null && "trim".equals(target) || target.endsWith(".trim")) { addViolation(data, node); } } return data; } - + } From 133f3d99dfbba2c3f42cae4f48dc9d84c7e9da30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Mart=C3=ADn=20Sotuyo=20Dodero?= Date: Wed, 22 Mar 2017 15:38:57 -0300 Subject: [PATCH 4/4] Update changelog --- src/site/markdown/overview/changelog.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/site/markdown/overview/changelog.md b/src/site/markdown/overview/changelog.md index 58b685e84a..bed96a3ac0 100644 --- a/src/site/markdown/overview/changelog.md +++ b/src/site/markdown/overview/changelog.md @@ -19,8 +19,11 @@ This is a bug fixing release. * java-design: * [#275](https://github.com/pmd/pmd/issues/275): \[java] FinalFieldCouldBeStatic: Constant in @interface incorrectly reported as "could be made static" +* java-strings: + * [#290](https://github.com/pmd/pmd/issues/290): \[java] InefficientEmptyStringCheck misses String.trim().isEmpty() ### API Changes ### External Contributions +* [#303](https://github.com/pmd/pmd/pull/303): \[java] InefficientEmptyStringCheckRule now reports String.trim().isEmpty()