From 43ecf1e110c531652f4e26666c050d4883aac2de Mon Sep 17 00:00:00 2001
From: Andreas Dangel <adangel@users.sourceforge.net>
Date: Sun, 17 Mar 2013 11:03:02 +0100
Subject: [PATCH] pmd: fix #999 Law of Demeter: False positives and negatives

---
 pmd/etc/changelog.txt                         |  1 +
 .../java/rule/coupling/LawOfDemeterRule.java  | 29 +++++++++---
 .../java/rule/coupling/xml/LawOfDemeter.xml   | 45 +++++++++++++++++++
 3 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/pmd/etc/changelog.txt b/pmd/etc/changelog.txt
index b9b929ab96..ea25e1e557 100644
--- a/pmd/etc/changelog.txt
+++ b/pmd/etc/changelog.txt
@@ -8,6 +8,7 @@ Fixed bug  984: Cyclomatic complexity should treat constructors like methods
 Fixed bug  985: Suppressed methods shouldn't affect avg CyclomaticComplexity
 Fixed bug  992: Class java.beans.Statement triggered in CloseResource rule
 Fixed bug  997: Rule NonThreadSafeSingleton gives analysis problem
+Fixed bug  999: Law of Demeter: False positives and negatives
 Fixed bug 1002: False +: FinalFieldCouldBeStatic on inner class
 Fixed bug 1005: False + for ConstructorCallsOverridableMethod - overloaded methods
 Fixed bug 1032: ImmutableField Rule: Private field in inner class gives false positive
diff --git a/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/coupling/LawOfDemeterRule.java b/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/coupling/LawOfDemeterRule.java
index 72d21eeee8..09244e4c9a 100644
--- a/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/coupling/LawOfDemeterRule.java
+++ b/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/coupling/LawOfDemeterRule.java
@@ -14,6 +14,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTAssignmentOperator;
 import net.sourceforge.pmd.lang.java.ast.ASTBlock;
 import net.sourceforge.pmd.lang.java.ast.ASTForStatement;
+import net.sourceforge.pmd.lang.java.ast.ASTLiteral;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTName;
 import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression;
@@ -123,13 +124,16 @@ public class LawOfDemeterRule extends AbstractJavaRule {
         public static List<MethodCall> createMethodCalls(ASTPrimaryExpression expression) {
             List<MethodCall> result = new ArrayList<MethodCall>();
 
-            if (isNotAConstructorCall(expression) && hasSuffixesWithArguments(expression)) {
+            if (isNotAConstructorCall(expression) && isNotLiteral(expression) && hasSuffixesWithArguments(expression)) {
                 ASTPrimaryPrefix prefixNode = expression.getFirstDescendantOfType(ASTPrimaryPrefix.class);
-                result.add(new MethodCall(expression, prefixNode));
+                MethodCall firstMethodCallInChain = new MethodCall(expression, prefixNode);
+                result.add(firstMethodCallInChain);
                 
-                List<ASTPrimarySuffix> suffixes = findSuffixesWithoutArguments(expression);
-                for (ASTPrimarySuffix suffix : suffixes) {
-                    result.add(new MethodCall(expression, suffix));
+                if (firstMethodCallInChain.isNotBuilder()) {
+                    List<ASTPrimarySuffix> suffixes = findSuffixesWithoutArguments(expression);
+                    for (ASTPrimarySuffix suffix : suffixes) {
+                        result.add(new MethodCall(expression, suffix));
+                    }
                 }
             }
             
@@ -140,6 +144,21 @@ public class LawOfDemeterRule extends AbstractJavaRule {
             return !expression.hasDescendantOfType(ASTAllocationExpression.class);
         }
 
+        private static boolean isNotLiteral(ASTPrimaryExpression expression) {
+            ASTPrimaryPrefix prefix = expression.getFirstDescendantOfType(ASTPrimaryPrefix.class);
+            if (prefix != null) {
+                return !prefix.hasDescendantOfType(ASTLiteral.class);
+            }
+            return true;
+        }
+
+        private boolean isNotBuilder() {
+            return baseType != StringBuffer.class
+                    && baseType != StringBuilder.class
+                    && !"StringBuilder".equals(baseTypeName)
+                    && !"StringBuffer".equals(baseTypeName);
+        }
+
         private static List<ASTPrimarySuffix> findSuffixesWithoutArguments(ASTPrimaryExpression expr) {
             List<ASTPrimarySuffix> result = new ArrayList<ASTPrimarySuffix>();
             if (hasRealPrefix(expr)) {
diff --git a/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/coupling/xml/LawOfDemeter.xml b/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/coupling/xml/LawOfDemeter.xml
index 34a6256f98..93ac6ef0c4 100644
--- a/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/coupling/xml/LawOfDemeter.xml
+++ b/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/coupling/xml/LawOfDemeter.xml
@@ -181,4 +181,49 @@ public class Foo {
 }
         </code>
     </test-code>
+
+    <test-code>
+        <description>#999 false positives</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Test {
+    //Thats no Violation in PMD:
+    public boolean compare1(final String aString) {
+        return aString.equals("S");
+    }
+
+    //Thats a Violation in PMD:
+    public boolean compare2(final String aString) {
+        return "A".equals(aString);              // < - - false positive
+    }
+
+    // no violation, because the object is not used
+    public Object create() {
+        Object o = myFactory.create();
+        return o;
+    }
+
+    // no violation, should be an exception, as the builder pattern is used here
+    public void toString() {
+        StringBuilder buffer = new StringBuilder();
+        buffer.append("string").append("string").append("string");
+        return buffer.toString();
+    }
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>#999 false negatives</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Test {
+    // violation, because object is used
+    public Object create2() {
+        Object o = myFactory.create();
+        o.setName("my name");                    // < - - !!!
+        return o;
+    }
+}
+        ]]></code>
+    </test-code>
 </test-data>