pmd: fix #999 Law of Demeter: False positives and negatives

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

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)) {

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>