phoedos/pmd
1
1
Fork 1
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

corrected invalid reporting of LoD violation

Browse Source
This commit is contained in:
Wayne Earl
2017-02-19 22:11:15 -07:00
committed by Juan Martín Sotuyo Dodero
parent acd891175d
commit 227060e5f8
2 changed files with 61 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

51
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/coupling/LawOfDemeterRule.java
View File

@ -41,7 +41,7 @@ import net.sourceforge.pmd.lang.symboltable.Scope;
* <li>Andrew Hunt, David Thomas, and Ward Cunningham. The Pragmatic Programmer. From Journeyman to Master. Addison-Wesley Longman, Amsterdam, October 1999.</li>
* <li>K.J. Lieberherr and I.M. Holland. Assuring good style for object-oriented programs. Software, IEEE, 6(5):38–48, 1989.</li>
* </ul>
*
*
* @since 5.0
*
*/
@ -51,7 +51,9 @@ public class LawOfDemeterRule extends AbstractJavaRule {
private static final String REASON_STATIC_ACCESS = "static property access";
/**
* That's a new method. We are going to check each method call inside the method.
* That's a new method. We are going to check each method call inside the
* method.
*
* @return <code>null</code>.
*/
@Override
@ -119,10 +121,10 @@ public class LawOfDemeterRule extends AbstractJavaRule {
}
/**
* Factory method to convert a given primary expression into MethodCalls.
* In case the primary expression represents a method chain call, then multiple
* MethodCalls are returned.
*
* Factory method to convert a given primary expression into
* MethodCalls. In case the primary expression represents a method chain
* call, then multiple MethodCalls are returned.
*
* @return a list of MethodCalls, might be empty.
*/
public static List<MethodCall> createMethodCalls(ASTPrimaryExpression expression) {
@ -307,12 +309,15 @@ public class LawOfDemeterRule extends AbstractJavaRule {
private Assignment determineLastAssignment() {
List<Assignment> assignments = new ArrayList<>();
ASTBlock block = expression.getFirstParentOfType(ASTMethodDeclaration.class).getFirstChildOfType(ASTBlock.class);
ASTBlock block = expression.getFirstParentOfType(ASTMethodDeclaration.class)
.getFirstChildOfType(ASTBlock.class);
//get all variableDeclarators within this block
List<ASTVariableDeclarator> variableDeclarators = block.findDescendantsOfType(ASTVariableDeclarator.class);
for (ASTVariableDeclarator declarator : variableDeclarators) {
ASTVariableDeclaratorId variableDeclaratorId = declarator.getFirstChildOfType(ASTVariableDeclaratorId.class);
ASTVariableDeclaratorId variableDeclaratorId = declarator
.getFirstChildOfType(ASTVariableDeclaratorId.class);
//we only care about it if the image name matches the current baseName
if (variableDeclaratorId.hasImageEqualTo(baseName)) {
boolean allocationFound = declarator.getFirstDescendantOfType(ASTAllocationExpression.class) != null;
boolean iterator = isIterator() || isFactory(declarator);
@ -320,18 +325,34 @@ public class LawOfDemeterRule extends AbstractJavaRule {
assignments.add(new Assignment(declarator.getBeginLine(), allocationFound, iterator, forLoop));
}
}
//get all AssignmentOperators within this block
List<ASTAssignmentOperator> assignmentStmts = block.findDescendantsOfType(ASTAssignmentOperator.class);
for (ASTAssignmentOperator stmt : assignmentStmts) {
if (stmt.hasImageEqualTo(SIMPLE_ASSIGNMENT_OPERATOR)) {
boolean allocationFound = stmt.jjtGetParent().getFirstDescendantOfType(ASTAllocationExpression.class) != null;
boolean iterator = isIterator();
assignments.add(new Assignment(stmt.getBeginLine(), allocationFound, iterator, false));
//we only care about it if it occurs prior to (or on) the beginLine of the current expression
//and if it is a simple_assignement_operator
if (stmt.getBeginLine() <= expression.getBeginLine()
&& stmt.hasImageEqualTo(SIMPLE_ASSIGNMENT_OPERATOR)) {
//now we need to make sure it has the right image name
ASTPrimaryPrefix primaryPrefix = stmt.jjtGetParent()
.getFirstDescendantOfType(ASTPrimaryPrefix.class);
if (primaryPrefix != null) {
ASTName prefixName = primaryPrefix.getFirstChildOfType(ASTName.class);
if (prefixName != null && prefixName.hasImageEqualTo(baseName)) {
//this is an assignment related to the baseName we are working with
boolean allocationFound = stmt.jjtGetParent()
.getFirstDescendantOfType(ASTAllocationExpression.class) != null;
boolean iterator = isIterator();
assignments
.add(new Assignment(stmt.getBeginLine(), allocationFound, iterator, false));
}
}
}
}
Assignment result = null;
if (!assignments.isEmpty()) {
//sort them in reverse order and return the first one
Collections.sort(assignments);
result = assignments.get(0);
}

35
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/coupling/xml/LawOfDemeter.xml
View File

@ -43,7 +43,7 @@ public class Foo {
}
]]></code>
</test-code>
<test-code>
<description>Simple Method calls with chaining</description>
<expected-problems>1</expected-problems>
@ -55,7 +55,7 @@ public class Foo {
}
]]></code>
</test-code>
<test-code>
<description>Simple Method calls with local created object</description>
<expected-problems>0</expected-problems>
@ -68,7 +68,22 @@ public class Foo {
}
]]></code>
</test-code>
<test-code>
<description>Simple Method calls with local created object and other variable assignment</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {
public void example() {
String something;
C c = new C();
c.doIt();
something = "no worries";
}
}
]]></code>
</test-code>
<test-code>
<description>Simple Method calls with local created object and variables</description>
<expected-problems>0</expected-problems>
@ -82,7 +97,7 @@ public class Foo {
}
]]></code>
</test-code>
<test-code>
<description>Simple Method call on local created object within nesting local scopes</description>
<expected-problems>0</expected-problems>
@ -99,7 +114,7 @@ public class Foo {
}
]]></code>
</test-code>
<test-code>
<description>Example documentation</description>
<expected-problems>2</expected-problems>
@ -111,18 +126,18 @@ public class Foo {
public void example(Bar b) {
// this method call is ok, as b is a parameter of "example"
C c = b.getC();
// this method call is a violation, as we are using c, which we got from B.
// We should ask b directly instead, e.g. "b.doItOnC();"
c.doIt();
// this is also a violation, just differently expressed as a method chain without temporary variables.
b.getC().doIt();
// that's a constructor call, not a method call.
D d = new D();
// this method call is ok, because we have create the new instance of D locally.
d.doSomethingElse();
d.doSomethingElse();
}
}
]]></code>
@ -170,7 +185,7 @@ public class Foo {
}
}
}
List&lt;String&gt; anotherList = calcList();
for (String s : anotherList) {
if (!s.isEmpty()) {