Add new Java Basic Rule: AvoidBranchingStatementAsLastInLoop
Cleanup changelog.txt, and the 50.xml. git-svn-id: https://pmd.svn.sourceforge.net/svnroot/pmd/trunk@6926 51baf565-9d33-0410-a72c-fc3788e3496d
This commit is contained in:
Ryan Gustafson committed
1 parent
11dddb5b87
commit
8199ab32d8
7 files changed
+345
-5
No files matched your search
@@ -469,21 +469,23 @@ AvoidUsingHardCodedIP modified to not use InetAddress.getByName(String), instead
|
||||
|
||||
New Java rules:
|
||||
|
||||
Basic ruleset: EmptyInitializer,EmptyStatementBlock,ExtendsObject,UselessParentheses,CheckSkipResult
|
||||
Controversial ruleset: AvoidLiteralsInIfCondition, AvoidPrefixingMethodParameters, OneDeclarationByLine
|
||||
Basic ruleset: ExtendsObject,CheckSkipResult,AvoidBranchingStatementAsLastInLoop
|
||||
Controversial ruleset: AvoidLiteralsInIfCondition, AvoidPrefixingMethodParameters, OneDeclarationPerLine
|
||||
Coupling ruleset: LoosePackageCoupling
|
||||
Design ruleset: LogicInversion,UseVarargs,FieldDeclarationsShouldBeAtStartOfClass
|
||||
Empty ruleset: EmptyInitializer,EmptyStatementBlock
|
||||
Import ruleset: UnnecessaryFullyQualifiedName
|
||||
Optimization ruleset: RedundantFieldInitializer
|
||||
Naming ruleset: ShortClassName
|
||||
StrictException ruleset: AvoidThrowingNewInstanceOfSameException, AvoidCatchingGenericException
|
||||
Unnecessary ruleset: UselessParentheses
|
||||
JUnit ruleset: JUnitTestContainsTooManyAsserts, UseAssertTrueInsteadOfAssertEquals
|
||||
|
||||
New Java ruleset:
|
||||
android.xml: new rules specific to the Android platform
|
||||
|
||||
New ECMAScript rules:
|
||||
Basic ruleset: AssignmentInOperand,InnaccurateNumericLiteral,UnreachableCode
|
||||
Basic ruleset: AssignmentInOperand,ConsistentReturn,InnaccurateNumericLiteral,UnreachableCode
|
||||
Braces ruleset: ForLoopsMustUseBraces,IfStmtsMustUseBraces,IfElseStmtsMustUseBraces,WhileLoopsMustUseBraces
|
||||
Unnecessary ruleset: UnnecessaryParentheses,UnnecessaryBlock
|
||||
|
||||
|
||||
@@ -13,6 +13,7 @@ public class BasicRulesTest extends SimpleAggregatorTst {
|
||||
|
||||
@Before
|
||||
public void setUp() {
|
||||
addRule(RULESET, "AvoidBranchingStatementAsLastInLoop");
|
||||
addRule(RULESET, "AvoidDecimalLiteralsInBigDecimalConstructor");
|
||||
addRule(RULESET, "AvoidMultipleUnaryOperators");
|
||||
addRule(RULESET, "AvoidThreadGroup");
|
||||
|
||||
+195
@@ -0,0 +1,195 @@
|
||||
<?xml version="1.0" encoding="UTF-8"?>
|
||||
<test-data>
|
||||
<code-fragment id="ok"><![CDATA[
|
||||
public class Foo {
|
||||
// These are all okay
|
||||
public void foo(boolean b) {
|
||||
for (int i = 0; i < 10;) {
|
||||
if (b) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
for (int i = 0; i < 10;) {
|
||||
if (b) {
|
||||
break;
|
||||
}
|
||||
}
|
||||
for (int i = 0; i < 10;) {
|
||||
if (b) {
|
||||
continue;
|
||||
}
|
||||
}
|
||||
while (true) {
|
||||
if (b) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
while (true) {
|
||||
if (b) {
|
||||
break;
|
||||
}
|
||||
}
|
||||
while (true) {
|
||||
if (b) {
|
||||
continue;
|
||||
}
|
||||
}
|
||||
do {
|
||||
if (b) {
|
||||
return true;
|
||||
}
|
||||
} while (true);
|
||||
do {
|
||||
if (b) {
|
||||
break;
|
||||
}
|
||||
} while (true);
|
||||
do {
|
||||
if (b) {
|
||||
continue;
|
||||
}
|
||||
} while (true);
|
||||
}
|
||||
}
|
||||
]]></code-fragment>
|
||||
<code-fragment id="violations"><![CDATA[
|
||||
public class Foo {
|
||||
// These are all bad
|
||||
public void bar() {
|
||||
for (int i = 0; i < 10;) {
|
||||
return true;
|
||||
}
|
||||
for (int i = 0; i < 10;) {
|
||||
break;
|
||||
}
|
||||
for (int i = 0; i < 10;) {
|
||||
continue;
|
||||
}
|
||||
while (true) {
|
||||
return true;
|
||||
}
|
||||
while (true) {
|
||||
break;
|
||||
}
|
||||
while (true) {
|
||||
continue;
|
||||
}
|
||||
do {
|
||||
return true;
|
||||
} while (true);
|
||||
do {
|
||||
break;
|
||||
} while (true);
|
||||
do {
|
||||
continue;
|
||||
} while (true);
|
||||
}
|
||||
}
|
||||
]]></code-fragment>
|
||||
<test-code>
|
||||
<description>ok: no violations</description>
|
||||
<expected-problems>0</expected-problems>
|
||||
<code-ref id="ok"/>
|
||||
</test-code>
|
||||
<test-code>
|
||||
<description>violations: break:for/do/while, continue:for/do/while and return:for/do/while</description>
|
||||
<expected-problems>9</expected-problems>
|
||||
<code-ref id="violations"/>
|
||||
</test-code>
|
||||
<test-code>
|
||||
<description>violations: break:for/do/while</description>
|
||||
<rule-property name="checkBreakLoopTypes">for|do|while</rule-property>
|
||||
<rule-property name="checkContinueLoopTypes"></rule-property>
|
||||
<rule-property name="checkReturnLoopTypes"></rule-property>
|
||||
<expected-problems>3</expected-problems>
|
||||
<code-ref id="violations"/>
|
||||
</test-code>
|
||||
<test-code>
|
||||
<description>violations: continue:for/do/while</description>
|
||||
<rule-property name="checkBreakLoopTypes"></rule-property>
|
||||
<rule-property name="checkContinueLoopTypes">for|do|while</rule-property>
|
||||
<rule-property name="checkReturnLoopTypes"></rule-property>
|
||||
<expected-problems>3</expected-problems>
|
||||
<code-ref id="violations"/>
|
||||
</test-code>
|
||||
<test-code>
|
||||
<description>violations: return:for/do/while</description>
|
||||
<rule-property name="checkBreakLoopTypes"></rule-property>
|
||||
<rule-property name="checkContinueLoopTypes"></rule-property>
|
||||
<rule-property name="checkReturnLoopTypes">for|do|while</rule-property>
|
||||
<expected-problems>3</expected-problems>
|
||||
<code-ref id="violations"/>
|
||||
</test-code>
|
||||
<test-code>
|
||||
<description>violations: break:for</description>
|
||||
<rule-property name="checkBreakLoopTypes">for</rule-property>
|
||||
<rule-property name="checkContinueLoopTypes"></rule-property>
|
||||
<rule-property name="checkReturnLoopTypes"></rule-property>
|
||||
<expected-problems>1</expected-problems>
|
||||
<code-ref id="violations"/>
|
||||
</test-code>
|
||||
<test-code>
|
||||
<description>violations: break:do</description>
|
||||
<rule-property name="checkBreakLoopTypes">do</rule-property>
|
||||
<rule-property name="checkContinueLoopTypes"></rule-property>
|
||||
<rule-property name="checkReturnLoopTypes"></rule-property>
|
||||
<expected-problems>1</expected-problems>
|
||||
<code-ref id="violations"/>
|
||||
</test-code>
|
||||
<test-code>
|
||||
<description>violations: break:while</description>
|
||||
<rule-property name="checkBreakLoopTypes">while</rule-property>
|
||||
<rule-property name="checkContinueLoopTypes"></rule-property>
|
||||
<rule-property name="checkReturnLoopTypes"></rule-property>
|
||||
<expected-problems>1</expected-problems>
|
||||
<code-ref id="violations"/>
|
||||
</test-code>
|
||||
<test-code>
|
||||
<description>violations: continue:for</description>
|
||||
<rule-property name="checkBreakLoopTypes"></rule-property>
|
||||
<rule-property name="checkContinueLoopTypes">for</rule-property>
|
||||
<rule-property name="checkReturnLoopTypes"></rule-property>
|
||||
<expected-problems>1</expected-problems>
|
||||
<code-ref id="violations"/>
|
||||
</test-code>
|
||||
<test-code>
|
||||
<description>violations: continue:do</description>
|
||||
<rule-property name="checkBreakLoopTypes"></rule-property>
|
||||
<rule-property name="checkContinueLoopTypes">do</rule-property>
|
||||
<rule-property name="checkReturnLoopTypes"></rule-property>
|
||||
<expected-problems>1</expected-problems>
|
||||
<code-ref id="violations"/>
|
||||
</test-code>
|
||||
<test-code>
|
||||
<description>violations: continue:while</description>
|
||||
<rule-property name="checkBreakLoopTypes"></rule-property>
|
||||
<rule-property name="checkContinueLoopTypes">while</rule-property>
|
||||
<rule-property name="checkReturnLoopTypes"></rule-property>
|
||||
<expected-problems>1</expected-problems>
|
||||
<code-ref id="violations"/>
|
||||
</test-code>
|
||||
<test-code>
|
||||
<description>violations: return:for</description>
|
||||
<rule-property name="checkBreakLoopTypes"></rule-property>
|
||||
<rule-property name="checkContinueLoopTypes"></rule-property>
|
||||
<rule-property name="checkReturnLoopTypes">for</rule-property>
|
||||
<expected-problems>1</expected-problems>
|
||||
<code-ref id="violations"/>
|
||||
</test-code>
|
||||
<test-code>
|
||||
<description>violations: return:do</description>
|
||||
<rule-property name="checkBreakLoopTypes"></rule-property>
|
||||
<rule-property name="checkContinueLoopTypes"></rule-property>
|
||||
<rule-property name="checkReturnLoopTypes">do</rule-property>
|
||||
<expected-problems>1</expected-problems>
|
||||
<code-ref id="violations"/>
|
||||
</test-code>
|
||||
<test-code>
|
||||
<description>violations: return:while</description>
|
||||
<rule-property name="checkBreakLoopTypes"></rule-property>
|
||||
<rule-property name="checkContinueLoopTypes"></rule-property>
|
||||
<rule-property name="checkReturnLoopTypes">while</rule-property>
|
||||
<expected-problems>1</expected-problems>
|
||||
<code-ref id="violations"/>
|
||||
</test-code>
|
||||
</test-data>
|
||||
@@ -31,6 +31,8 @@
|
||||
|
||||
<rule ref="rulesets/java/basic.xml">
|
||||
<exclude name="CollapsibleIfStatements"/>
|
||||
<!-- TODO Work towards enabling the following, disabled for now because of failing code -->
|
||||
<exclude name="AvoidBranchingStatementAsLastInLoop"/>
|
||||
</rule>
|
||||
|
||||
<rule ref="rulesets/java/braces.xml"/>
|
||||
|
||||
@@ -743,6 +743,42 @@ public class Foo {
|
||||
</example>
|
||||
</rule>
|
||||
|
||||
<rule name="AvoidBranchingStatementAsLastInLoop"
|
||||
since="5.0"
|
||||
class="net.sourceforge.pmd.lang.java.rule.basic.AvoidBranchingStatementAsLastInLoopRule"
|
||||
message="Avoid using a branching statement as the last in a loop."
|
||||
externalInfoUrl="http://pmd.sourceforge.net/rules/java/basic.html#AvoidBranchingStatementAsLastInLoop">
|
||||
<description>
|
||||
<![CDATA[
|
||||
Using a branching statement as the last in a loop may be a bug, and/or is confusing.
|
||||
Check the usage is not a bug, or consider changing.
|
||||
]]>
|
||||
</description>
|
||||
<priority>2</priority>
|
||||
<example>
|
||||
<![CDATA[
|
||||
public class Foo {
|
||||
public void bar() {
|
||||
// Funky use of branching statement in a loop...
|
||||
for (int i = 0; i < 10; i++) {
|
||||
if (i*i <= 25) {
|
||||
continue;
|
||||
}
|
||||
break;
|
||||
}
|
||||
|
||||
// This makes more sense...
|
||||
for (int i = 0; i < 10; i++) {
|
||||
if (i*i > 25) {
|
||||
break;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
]]>
|
||||
</example>
|
||||
</rule>
|
||||
|
||||
<rule deprecated="true" ref="rulesets/java/empty.xml/EmptyCatchBlock" />
|
||||
<rule deprecated="true" ref="rulesets/java/empty.xml/EmptyIfStmt" />
|
||||
<rule deprecated="true" ref="rulesets/java/empty.xml/EmptyWhileStmt" />
|
||||
|
||||
@@ -6,10 +6,25 @@
|
||||
This ruleset contains links to rules that are new in PMD v5.0
|
||||
</description>
|
||||
|
||||
<rule ref="rulesets/java/android.xml"/>
|
||||
<rule ref="rulesets/ecmascript/basic.xml/AssignmentInOperand"/>
|
||||
<rule ref="rulesets/ecmascript/basic.xml/ConsistentReturn"/>
|
||||
<rule ref="rulesets/ecmascript/basic.xml/InnaccurateNumericLiteral"/>
|
||||
<rule ref="rulesets/ecmascript/basic.xml/UnreachableCode"/>
|
||||
<rule ref="rulesets/ecmascript/braces.xml/ForLoopsMustUseBraces"/>
|
||||
<rule ref="rulesets/ecmascript/braces.xml/IfStmtsMustUseBraces"/>
|
||||
<rule ref="rulesets/ecmascript/braces.xml/IfElseStmtsMustUseBraces"/>
|
||||
<rule ref="rulesets/ecmascript/braces.xml/WhileLoopsMustUseBraces"/>
|
||||
<rule ref="rulesets/ecmascript/unnecessary.xml/UnnecessaryParentheses"/>
|
||||
<rule ref="rulesets/ecmascript/unnecessary.xml/UnnecessaryBlock"/>
|
||||
|
||||
<rule ref="rulesets/java/android.xml/CallSuperFirst"/>
|
||||
<rule ref="rulesets/java/android.xml/CallSuperLast"/>
|
||||
<rule ref="rulesets/java/android.xml/ProtectLogD"/>
|
||||
<rule ref="rulesets/java/android.xml/ProtectLogV"/>
|
||||
<rule ref="rulesets/java/basic.xml/CheckSkipResult"/>
|
||||
<rule ref="rulesets/java/basic.xml/ExtendsObject"/>
|
||||
<rule ref="rulesets/java/controversial.xml/AvoidPrefixingMethodParameters"/>
|
||||
<rule ref="rulesets/java/controversial.xml/AvoidLiteralsInIfCondition"/>
|
||||
<rule ref="rulesets/java/controversial.xml/OneDeclarationPerLine"/>
|
||||
<rule ref="rulesets/java/coupling.xml/LoosePackageCoupling"/>
|
||||
<rule ref="rulesets/java/design.xml/FieldDeclarationsShouldBeAtStartOfClass"/>
|
||||
@@ -17,11 +32,15 @@ This ruleset contains links to rules that are new in PMD v5.0
|
||||
<rule ref="rulesets/java/design.xml/UseVarargs"/>
|
||||
<rule ref="rulesets/java/empty.xml/EmptyInitializer"/>
|
||||
<rule ref="rulesets/java/empty.xml/EmptyStatementBlock"/>
|
||||
<rule ref="rulesets/java/imports.xml/UnnecessaryFullyQualifiedName"/>
|
||||
<rule ref="rulesets/java/imports.xml/UnnecessaryFullyQualifiedName"/>
|
||||
<rule ref="rulesets/java/junit.xml/JUnitTestContainsTooManyAsserts"/>
|
||||
<rule ref="rulesets/java/junit.xml/UseAssertTrueInsteadOfAssertEquals"/>
|
||||
<rule ref="rulesets/java/naming.xml/ShortClassName"/>
|
||||
<rule ref="rulesets/java/optimizations.xml/RedundantFieldInitializer"/>
|
||||
<rule ref="rulesets/java/strictexception.xml/AvoidCatchingGenericException"/>
|
||||
<rule ref="rulesets/java/strictexception.xml/AvoidThrowingNewInstanceOfSameException"/>
|
||||
<rule ref="rulesets/java/unnecessary.xml/UselessParentheses"/>
|
||||
|
||||
<rule ref="rulesets/xml/basic.xml/MistypedCDATASection"/>
|
||||
</ruleset>
|
||||
|
||||
+85
@@ -0,0 +1,85 @@
|
||||
package net.sourceforge.pmd.lang.java.rule.basic;
|
||||
|
||||
import net.sourceforge.pmd.lang.ast.Node;
|
||||
import net.sourceforge.pmd.lang.java.ast.ASTBreakStatement;
|
||||
import net.sourceforge.pmd.lang.java.ast.ASTContinueStatement;
|
||||
import net.sourceforge.pmd.lang.java.ast.ASTDoStatement;
|
||||
import net.sourceforge.pmd.lang.java.ast.ASTForStatement;
|
||||
import net.sourceforge.pmd.lang.java.ast.ASTReturnStatement;
|
||||
import net.sourceforge.pmd.lang.java.ast.ASTWhileStatement;
|
||||
import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
|
||||
import net.sourceforge.pmd.lang.rule.properties.EnumeratedMultiProperty;
|
||||
|
||||
public class AvoidBranchingStatementAsLastInLoopRule extends AbstractJavaRule {
|
||||
|
||||
public static final String CHECK_FOR = "for";
|
||||
public static final String CHECK_DO = "do";
|
||||
public static final String CHECK_WHILE = "while";
|
||||
|
||||
private static final String[] ALL_LOOP_TYPES_LABELS = new String[] { CHECK_FOR, CHECK_DO, CHECK_WHILE };
|
||||
private static final String[] ALL_LOOP_TYPES_VALUES = ALL_LOOP_TYPES_LABELS;
|
||||
private static final int[] ALL_LOOP_TYPES_DEFAULTS = new int[] { 0, 1, 2 };
|
||||
|
||||
public static final EnumeratedMultiProperty<String> CHECK_BREAK_LOOP_TYPES = new EnumeratedMultiProperty(
|
||||
"checkBreakLoopTypes", "Check for break statements in loop types", ALL_LOOP_TYPES_LABELS,
|
||||
ALL_LOOP_TYPES_VALUES, ALL_LOOP_TYPES_DEFAULTS, 1);
|
||||
public static final EnumeratedMultiProperty<String> CHECK_CONTINUE_LOOP_TYPES = new EnumeratedMultiProperty(
|
||||
"checkContinueLoopTypes", "Check for continue statements in loop types", ALL_LOOP_TYPES_LABELS,
|
||||
ALL_LOOP_TYPES_VALUES, ALL_LOOP_TYPES_DEFAULTS, 2);
|
||||
public static final EnumeratedMultiProperty<String> CHECK_RETURN_LOOP_TYPES = new EnumeratedMultiProperty(
|
||||
"checkReturnLoopTypes", "Check for return statements in loop types", ALL_LOOP_TYPES_LABELS,
|
||||
ALL_LOOP_TYPES_VALUES, ALL_LOOP_TYPES_DEFAULTS, 3);
|
||||
|
||||
public AvoidBranchingStatementAsLastInLoopRule() {
|
||||
definePropertyDescriptor(CHECK_BREAK_LOOP_TYPES);
|
||||
definePropertyDescriptor(CHECK_CONTINUE_LOOP_TYPES);
|
||||
definePropertyDescriptor(CHECK_RETURN_LOOP_TYPES);
|
||||
|
||||
addRuleChainVisit(ASTBreakStatement.class);
|
||||
addRuleChainVisit(ASTContinueStatement.class);
|
||||
addRuleChainVisit(ASTReturnStatement.class);
|
||||
}
|
||||
|
||||
@Override
|
||||
public Object visit(ASTBreakStatement node, Object data) {
|
||||
return check(CHECK_BREAK_LOOP_TYPES, node, data);
|
||||
}
|
||||
|
||||
@Override
|
||||
public Object visit(ASTContinueStatement node, Object data) {
|
||||
return check(CHECK_CONTINUE_LOOP_TYPES, node, data);
|
||||
}
|
||||
|
||||
@Override
|
||||
public Object visit(ASTReturnStatement node, Object data) {
|
||||
return check(CHECK_RETURN_LOOP_TYPES, node, data);
|
||||
}
|
||||
|
||||
protected Object check(EnumeratedMultiProperty<String> property, Node node, Object data) {
|
||||
Node parent = node.getNthParent(5);
|
||||
if (parent instanceof ASTForStatement) {
|
||||
if (hasPropertyValue(property, CHECK_FOR)) {
|
||||
super.addViolation(data, node);
|
||||
}
|
||||
} else if (parent instanceof ASTWhileStatement) {
|
||||
if (hasPropertyValue(property, CHECK_WHILE)) {
|
||||
super.addViolation(data, node);
|
||||
}
|
||||
} else if (parent instanceof ASTDoStatement) {
|
||||
if (hasPropertyValue(property, CHECK_DO)) {
|
||||
super.addViolation(data, node);
|
||||
}
|
||||
}
|
||||
return data;
|
||||
}
|
||||
|
||||
protected boolean hasPropertyValue(EnumeratedMultiProperty<String> property, String value) {
|
||||
final Object[] values = getProperty(property);
|
||||
for (int i = 0; i < values.length; i++) {
|
||||
if (value.equals(values[i])) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
return false;
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user