[java] UnnecessaryLocalBeforeReturn checks usages
- We now check symbol table usages to detect violations. - We drop the consecutive lines requirement. - Resolves #240
This commit is contained in:

committed by
Andreas Dangel

parent
e3abbcbdd5
commit
4137df4309
@ -6,9 +6,6 @@ package net.sourceforge.pmd.lang.java.rule.design;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
|
||||
import net.sourceforge.pmd.lang.ast.Node;
|
||||
import net.sourceforge.pmd.lang.java.ast.ASTAssertStatement;
|
||||
import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement;
|
||||
import net.sourceforge.pmd.lang.java.ast.ASTExpression;
|
||||
import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
|
||||
import net.sourceforge.pmd.lang.java.ast.ASTName;
|
||||
@ -43,26 +40,20 @@ public class UnnecessaryLocalBeforeReturnRule extends AbstractJavaRule {
|
||||
return data;
|
||||
}
|
||||
|
||||
Map<VariableNameDeclaration, List<NameOccurrence>> vars = name.getScope().getDeclarations(VariableNameDeclaration.class);
|
||||
for (Map.Entry<VariableNameDeclaration, List<NameOccurrence>> entry: vars.entrySet()) {
|
||||
VariableNameDeclaration key = entry.getKey();
|
||||
Map<VariableNameDeclaration, List<NameOccurrence>> vars = name.getScope()
|
||||
.getDeclarations(VariableNameDeclaration.class);
|
||||
for (Map.Entry<VariableNameDeclaration, List<NameOccurrence>> entry : vars.entrySet()) {
|
||||
List<NameOccurrence> usages = entry.getValue();
|
||||
|
||||
// skip, if there is an assert between declaration and return
|
||||
if (hasAssertStatement(key, rtn)) {
|
||||
continue;
|
||||
}
|
||||
if (usages.size() == 1) { // If there is more than 1 usage, then it's not only returned
|
||||
NameOccurrence occ = usages.get(0);
|
||||
|
||||
for (NameOccurrence occ: usages) {
|
||||
if (occ.getLocation().equals(name)) {
|
||||
// only check declarations that occur one line earlier
|
||||
if (key.getNode().getBeginLine() == name.getBeginLine() - 1) {
|
||||
String var = name.getImage();
|
||||
if (var.indexOf('.') != -1) {
|
||||
var = var.substring(0, var.indexOf('.'));
|
||||
}
|
||||
addViolation(data, rtn, var);
|
||||
String var = name.getImage();
|
||||
if (var.indexOf('.') != -1) {
|
||||
var = var.substring(0, var.indexOf('.'));
|
||||
}
|
||||
addViolation(data, rtn, var);
|
||||
}
|
||||
}
|
||||
}
|
||||
@ -85,31 +76,4 @@ public class UnnecessaryLocalBeforeReturnRule extends AbstractJavaRule {
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
/**
|
||||
* Checks whether there is an assert statement between the variable declaration
|
||||
* and the return statement, that uses the variable.
|
||||
* @param variableDeclaration
|
||||
* @param rtn
|
||||
* @return
|
||||
*/
|
||||
private boolean hasAssertStatement(VariableNameDeclaration variableDeclaration, ASTReturnStatement rtn) {
|
||||
ASTBlockStatement blockStatement = variableDeclaration.getAccessNodeParent().getFirstParentOfType(ASTBlockStatement.class);
|
||||
int startIndex = blockStatement.jjtGetChildIndex() + 1;
|
||||
int endIndex = rtn.getFirstParentOfType(ASTBlockStatement.class).jjtGetChildIndex();
|
||||
Node block = blockStatement.jjtGetParent();
|
||||
for (int i = startIndex; i < endIndex; i++) {
|
||||
List<ASTAssertStatement> asserts = block.jjtGetChild(i).findDescendantsOfType(ASTAssertStatement.class);
|
||||
for (ASTAssertStatement assertStatement : asserts) {
|
||||
List<ASTName> names = assertStatement.findDescendantsOfType(ASTName.class);
|
||||
for (ASTName n : names) {
|
||||
if (n.hasImageEqualTo(variableDeclaration.getName())) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
@ -91,6 +91,38 @@ public class Foo {
|
||||
return i;
|
||||
}
|
||||
}
|
||||
}
|
||||
]]></code>
|
||||
</test-code>
|
||||
|
||||
<test-code>
|
||||
<description>Detect violation even if not on consecutive lines</description>
|
||||
<expected-problems>1</expected-problems>
|
||||
<code><![CDATA[
|
||||
public class Foo {
|
||||
public int bar() {
|
||||
int res = 2;
|
||||
doSomething();
|
||||
return res;
|
||||
}
|
||||
|
||||
public void doSomething() { }
|
||||
}
|
||||
]]></code>
|
||||
</test-code>
|
||||
|
||||
<test-code>
|
||||
<description>No violations on multiple uses of the variable</description>
|
||||
<expected-problems>0</expected-problems>
|
||||
<code><![CDATA[
|
||||
public class Foo {
|
||||
public int bar() {
|
||||
int res = 2;
|
||||
doSomething(res);
|
||||
return res;
|
||||
}
|
||||
|
||||
public void doSomething(int x) { }
|
||||
}
|
||||
]]></code>
|
||||
</test-code>
|
||||
|
Reference in New Issue
Block a user