Added new AvoidRethrowingException rule; thanks to George Thomas for the contribution!

git-svn-id: https://pmd.svn.sourceforge.net/svnroot/pmd/trunk@4524 51baf565-9d33-0410-a72c-fc3788e3496d
2006-09-22 13:14:31 +00:00
parent 59a61d92a2
commit 785b64e8e6
6 changed files with 121 additions and 43 deletions
--- a/pmd/etc/changelog.txt
+++ b/pmd/etc/changelog.txt
@ -4,6 +4,7 @@ BEFORE RELEASE:

 New rules:
    Basic ruleset: BrokenNullCheck
+    Strict exceptions ruleset: AvoidRethrowingException
 Fixed bug 1498910 - AssignmentInOperand no longer has a typo in the message.
 Fixed bug 1498960 - DontImportJavaLang no longer reports static imports of java.lang members.
 Fixed bug 1417106 - MissingBreakInSwitch no longer flags stmts where every case has either a return or a throw.
--- a/pmd/regress/test/net/sourceforge/pmd/rules/strictexception/AvoidRethrowingExceptionTest.java
+++ b/pmd/regress/test/net/sourceforge/pmd/rules/strictexception/AvoidRethrowingExceptionTest.java
@ -0,0 +1,55 @@
+package test.net.sourceforge.pmd.rules.strictexception;
+
+import net.sourceforge.pmd.PMD;
+import net.sourceforge.pmd.Rule;
+import net.sourceforge.pmd.RuleSetNotFoundException;
+import test.net.sourceforge.pmd.testframework.SimpleAggregatorTst;
+import test.net.sourceforge.pmd.testframework.TestDescriptor;
+
+/**
+ * Tests the <code>AvoidRethrowingException</code> rule.
+ *
+ * @author George Thomas
+ */
+public class AvoidRethrowingExceptionTest extends SimpleAggregatorTst {
+
+    private Rule rule;
+
+    /**
+     * @see SimpleAggregatorTst#setUp()
+     */
+    public void setUp() throws RuleSetNotFoundException {
+        rule = findRule("strictexception", "AvoidRethrowingException");
+    }
+
+    /**
+     * @see SimpleAggregatorTst#testAll()
+     */
+    public void testAll() {
+        runTests(new TestDescriptor[] {
+            new TestDescriptor(FAILURE_TEST, "failure case", 1, rule),
+            new TestDescriptor(OK_TEST, "doing something else before throwing it, ok", 0, rule),
+        });
+    }
+
+    private static final String FAILURE_TEST =
+            "public class Foo {" + PMD.EOL
+            + "  void bar() {" + PMD.EOL
+            + "   try {" + PMD.EOL
+            + "   } catch (SomeException se) {" + PMD.EOL
+            + "    throw se;" + PMD.EOL
+            + "   }" + PMD.EOL
+            + "  }" + PMD.EOL
+            + "} " + PMD.EOL;
+
+    private static final String OK_TEST =
+            "public class Foo {" + PMD.EOL
+            + "  void bar() {" + PMD.EOL
+            + "   try {" + PMD.EOL
+            + "   } catch (SomeException se) {" + PMD.EOL
+            + "    System.out.println(\"something interesting\");" + PMD.EOL
+            + "    throw se;" + PMD.EOL
+            + "   }" + PMD.EOL
+            + "  }" + PMD.EOL
+            + "} " + PMD.EOL;
+}
--- a/pmd/rulesets/releases/38.xml
+++ b/pmd/rulesets/releases/38.xml
@ -7,5 +7,6 @@ This ruleset contains links to rules that are new in PMD v3.8
  </description>

  <rule ref="rulesets/basic.xml/BrokenNullCheck"/>
+  <rule ref="rulesets/strictexception.xml/AvoidRethrowingException"/>
 </ruleset>

--- a/pmd/rulesets/scratchpad.xml
+++ b/pmd/rulesets/scratchpad.xml
@ -10,52 +10,40 @@ These are new rules that are still in progress
  </description>


-    <rule name="BrokenNullCheck"
-          message="Method call on object which may be null"
-          class="net.sourceforge.pmd.rules.XPathRule"
-      externalInfoUrl="http://pmd.sourceforge.net/rules/basic.html#BrokenNullCheck">
-        <description>
-The null check is broken since it will throw a Nullpointer itself.
-The reason is that a method is called on the object when it is null.
-It is likely that you used || instead of &amp;&amp; or vice versa.
-     </description>
-        <properties>
-            <property name="xpath">
-                <value><![CDATA[
-//IfStatement/Expression[.//NullLiteral]
-[
-.//ConditionalOrExpression/EqualityExpression[@Image='!=']
-[starts-with(../*[position()>1]//PrimaryPrefix/Name/@Image,
-concat(PrimaryExpression//PrimaryPrefix/Name/@Image, '.'))]
-or
-.//ConditionalAndExpression/EqualityExpression[@Image='==']
-[starts-with(../*[position()>1]//PrimaryPrefix/Name/@Image,
-concat(PrimaryExpression//PrimaryPrefix/Name/@Image, '.'))]
-]
-                ]]>
-                </value>
-            </property>
-        </properties>
-        <priority>2</priority>
-        <example>
-<![CDATA[
-class Foo {
- String munge(String string) {
-  // should be &&
-  if (string!=null || !string.equals(""))
-    return string;
-  // should be ||
-  if (string==null && string.equals(""))
-    return string;
- }
-}
-        ]]>
-        </example>
+    <rule name="AvoidRethrowingException"
+      message="A catch statement that catches an exception only to rethrow it should be avoided"
+       class="net.sourceforge.pmd.rules.XPathRule">
+      <description>
+       Catch blocks that merely rethrow a caught exception only add to code size and runtime complexity.
+      </description>
+      <example>  <![CDATA[
+    public class Foo {
+     void bar() {
+      try {
+      // do something
+      }  catch (SomeException se) {
+         throw se;
+      }
+     }
+    }
+    ]]>
+      </example>
+      <priority> 3 </priority>
+      <properties>
+      <property name="xpath">
+        <value>
+          <![CDATA[
+  //CatchStatement[FormalParameter
+   /VariableDeclaratorId = Block/BlockStatement/Statement
+   /ThrowStatement/Expression/PrimaryExpression/PrimaryPrefix/Name
+   and count(Block/BlockStatement/Statement) =1]
+   ]]>
+        </value>
+      </property>
+      </properties>
    </rule>


-
-
    <!--
    <rule name="UselessAssignment"
           message="This assignment to ''{0}'' is useless"
--- a/pmd/rulesets/strictexception.xml
+++ b/pmd/rulesets/strictexception.xml
@ -172,4 +172,36 @@ public class Foo {
      </properties>
    </rule>

+  <rule name="AvoidRethrowingException"
+    message="A catch statement that catches an exception only to rethrow it should be avoided"
+     class="net.sourceforge.pmd.rules.XPathRule">
+    <description>
+     Catch blocks that merely rethrow a caught exception only add to code size and runtime complexity.
+    </description>
+    <example>  <![CDATA[
+  public class Foo {
+   void bar() {
+    try {
+    // do something
+    }  catch (SomeException se) {
+       throw se;
+    }
+   }
+  }
+  ]]>
+    </example>
+    <priority> 3 </priority>
+    <properties>
+    <property name="xpath">
+      <value>
+        <![CDATA[
+//CatchStatement[FormalParameter
+ /VariableDeclaratorId = Block/BlockStatement/Statement
+ /ThrowStatement/Expression/PrimaryExpression/PrimaryPrefix/Name
+ and count(Block/BlockStatement/Statement) =1]
+ ]]>
+      </value>
+    </property>
+    </properties>
+  </rule>
 </ruleset>
--- a/pmd/xdocs/credits.xml
+++ b/pmd/xdocs/credits.xml
@ -51,6 +51,7 @@
        </subsection>
        <subsection name="Contributors">
        <ul>
+            <li>George Thomas - Wrote AvoidRethrowingException rule</li>
            <li>Robert Simmons - Reported bug in optimizations package along with suggestions for fix</li>
            <li>Xavier Le Vourch - Patch to fix speling in CouplingBetweenObjects message, patch to improve CloneMethodMustImplementCloneable, suggestions on cleaning up casts in grammar, SimplifyBooleanAssertion, patch to fix problem with TestClassWithoutTestCases, patch to fix rule name bugs in migration rulesets</li>
            <li>Brian Remedios - display cleanup of CPD GUI, code cleanup of StringUtil and various rules, cleanup of rule designer, code cleanup of net.sourceforge.pmd.ant.Formatter.java, code improvements to Eclipse plugin, created AbstractPoorMethodCall and refactored UseIndexOfChar</li>