Merge pull request #517 from Zetten/NestedStatementCheck

Add new check for nesting level

cxx-checks/src/main/java/org/sonar/cxx/checks/CheckList.java

@@ -73,7 +73,8 @@ public static List<Class> getChecks() {
       UseCorrectIncludeCheck.class,
       XPathCheck.class,
       BooleanEqualityComparisonCheck.class,
-      CompileIncludePathNotFoundOrInvalid.class
+      CompileIncludePathNotFoundOrInvalid.class,
+      NestedStatementsCheck.class
     );
   }
 

cxx-checks/src/main/java/org/sonar/cxx/checks/NestedStatementsCheck.java

@@ -0,0 +1,120 @@
+/*
+ * Sonar C++ Plugin (Community)
+ * Copyright (C) 2011 Waleri Enns and CONTACT Software GmbH
+ * [email protected]
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02
+ */
+package org.sonar.cxx.checks;
+
+
+import com.google.common.collect.Sets;
+import com.sonar.sslr.api.AstNode;
+import com.sonar.sslr.api.AstNodeType;
+import com.sonar.sslr.api.Grammar;
+import org.sonar.api.server.rule.RulesDefinition;
+import org.sonar.check.Priority;
+import org.sonar.check.Rule;
+import org.sonar.check.RuleProperty;
+import org.sonar.cxx.parser.CxxGrammarImpl;
+import org.sonar.squidbridge.annotations.ActivatedByDefault;
+import org.sonar.squidbridge.annotations.SqaleConstantRemediation;
+import org.sonar.squidbridge.annotations.SqaleSubCharacteristic;
+import org.sonar.squidbridge.annotations.Tags;
+import org.sonar.squidbridge.checks.SquidCheck;
+
+import java.util.List;
+import java.util.Set;
+
+@Rule(
+  key = "NestedStatements",
+  name = "Control flow statements \"if\", \"switch\", \"try\" and iterators should not be nested too deeply",
+  tags = { Tags.BRAIN_OVERLOAD },
+  priority = Priority.MAJOR
+)
+@ActivatedByDefault
+@SqaleSubCharacteristic(RulesDefinition.SubCharacteristics.LOGIC_CHANGEABILITY)
+@SqaleConstantRemediation("10min")
+public class NestedStatementsCheck extends SquidCheck<Grammar> {
+
+  private static final AstNodeType[] CHECKED_TYPES = new AstNodeType[]{
+    CxxGrammarImpl.ifStatement,
+    CxxGrammarImpl.switchStatement,
+    CxxGrammarImpl.tryBlock,
+    CxxGrammarImpl.iterationStatement
+  };
+
+  private static final int DEFAULT_MAX = 3;
+
+  private static final String ELSE_TOKEN = "ELSE";
+
+  @RuleProperty(defaultValue = "" + DEFAULT_MAX,
+      description = "Maximum allowed control flow statement nesting depth.")
+  public int max = DEFAULT_MAX;
+
+  private Set<AstNode> checkedNodes = Sets.newHashSet();
+  private int nestingLevel = 0;
+
+  @Override
+  public void init() {
+    subscribeTo(CHECKED_TYPES);
+  }
+
+  @Override
+  public void visitNode(AstNode node) {
+    if (checkedNodes.contains(node)) {
+      return;
+    }
+
+    List<AstNode> watchedDescendants = node.getDescendants(CHECKED_TYPES);
+
+    // In the AST 'else if' blocks are technically nested, but should not increase the nesting level as they are
+    // actually flat in terms of 'spaghetti code'. This bypasses the nesting increment/decrement for such blocks.
+    if (isElseIf(node)) {
+      visitChildren(watchedDescendants);
+    } else {
+      nestingLevel++;
+
+      // If the max level is reached, stop descending the tree and create a violation
+      if (nestingLevel == max + 1) {
+        getContext().createLineViolation(
+            this,
+            "Refactor this code to not nest more than " + max + " if/switch/try/for/while/do statements.",
+            node);
+      } else {
+        visitChildren(watchedDescendants);
+      }
+
+      nestingLevel--;
+    }
+
+    // Prevent re-checking of descendent nodes
+    checkedNodes.addAll(watchedDescendants);
+  }
+
+  private void visitChildren(List<AstNode> watchedDescendants) {
+    for (AstNode descendant : watchedDescendants) {
+      visitNode(descendant);
+    }
+  }
+
+  /**
+   * @return True if the given node is the 'if' in an 'else if' construct.
+   */
+  private boolean isElseIf(AstNode node) {
+    return node.getType() == CxxGrammarImpl.ifStatement
+      && node.getParent().getPreviousAstNode().getType().toString().equals(ELSE_TOKEN);
+  }
+}

cxx-checks/src/main/resources/org/sonar/l10n/c++/rules/cxx/NestedStatements.html

@@ -0,0 +1,36 @@
+<p>
+Nested <code>if</code>, <code>switch</code>, <code>try</code> and iterator (<code>for</code>, <code>while</code>,
+<code>do</code>) statements are a key ingredient for making what's known as "Spaghetti code". Such code is hard to read,
+refactor and therefore maintain.
+</p>
+
+<h2>Noncompliant Code Example</h2>
+
+<p>The following code snippet illustrates this rule with the default threshold of 3.</p>
+
+<pre>
+class Abc {
+  public:
+    void noncompliant( ) {
+      if (true) {
+        try {
+          if (true) {
+            for (int i = 0; i < 1; i++) {
+              printf("The enclosing block is non-compliant.");
+            }
+          }
+        } catch (...) {
+          do {
+            if (true) {
+              printf("The enclosing block is non-compliant.");
+
+              if (true) {
+                printf("The enclosing block exceeds the limit, but issues are only reported on depth = 4.");
+              }
+            }
+          } while (true);
+        }
+      }
+    }
+};
+</pre>

cxx-checks/src/test/java/org/sonar/cxx/checks/CheckListTest.java

@@ -27,6 +27,6 @@ public class CheckListTest {
 
   @Test
   public void count() {
-    assertThat(CheckList.getChecks().size()).isEqualTo(40);
+    assertThat(CheckList.getChecks().size()).isEqualTo(41);
   }
 }

cxx-checks/src/test/java/org/sonar/cxx/checks/NestedStatementsCheckTest.java

@@ -0,0 +1,48 @@
+/*
+ * Sonar C++ Plugin (Community)
+ * Copyright (C) 2011 Waleri Enns and CONTACT Software GmbH
+ * [email protected]
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02
+ */
+package org.sonar.cxx.checks;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.sonar.cxx.CxxAstScanner;
+import org.sonar.squidbridge.api.CheckMessage;
+import org.sonar.squidbridge.api.SourceFile;
+import org.sonar.squidbridge.checks.CheckMessagesVerifier;
+import org.sonar.squidbridge.checks.CheckMessagesVerifierRule;
+
+import java.io.File;
+
+public class NestedStatementsCheckTest {
+
+  @Rule
+  public CheckMessagesVerifierRule checkMessagesVerifier = new CheckMessagesVerifierRule();
+
+  @Test
+  public void detected() {
+    NestedStatementsCheck check = new NestedStatementsCheck();
+    check.max = 5;
+    SourceFile file = CxxAstScanner.scanSingleFile(new File("src/test/resources/checks/NestedStatementsCheck.cc"), check);
+    checkMessagesVerifier.verify(file.getCheckMessages())
+      .next().atLine(62)
+      .next().atLine(67)
+      .noMore();
+  }
+
+}

cxx-checks/src/test/resources/checks/NestedStatementsCheck.cc

@@ -0,0 +1,79 @@
+class Abc {
+  public:
+
+    void compliant() {
+      int i = 0;
+
+      if (true) {
+        for (int i = 0; i < 1; i++) {
+          if (true) {
+            if (true) {
+              printf("Four levels deep; compliant.");
+            }
+          } else if (true) {
+            switch (i) {
+              case 0:
+                printf("Five levels deep; compliant.");
+                break;
+              default:
+                break;
+            }
+          } else if (true) {
+            printf("Three levels deep; compliant.");
+          } else if (true) {
+            printf("Three levels deep; compliant.");
+          } else {
+            printf("Three levels deep; compliant.");
+          }
+        }
+      }
+
+      if (true) {
+        printf("One level deep; compliant.");
+      } else if (true) {
+        printf("One level deep; compliant.");
+      } else if (true) {
+        printf("One level deep; compliant.");
+      } else if (true) {
+        printf("One level deep; compliant.");
+      } else if (true) {
+        printf("One level deep; compliant.");
+      } else if (true) {
+        printf("One level deep; compliant.");
+      }
+    }
+
+    void noncompliant( ) {
+      int i = 0;
+
+      if (true) {
+        for (int i = 0; i < 1; i++) {
+          do {
+            if (true) {
+              printf("Four levels deep; compliant.");
+            } else if (true) {
+              printf("Four levels deep; compliant.");
+            } else {
+              switch (i) {
+                case 0:
+                  printf("Five levels deep; compliant.");
+                  break;
+                case 1:
+                  if (true) {
+                    printf("Six levels deep; non-compliant.");
+                  }
+                  break;
+                default:
+                  try {
+                    printf("Six levels deep; non-compliant.");
+                  } catch (Exception e) {
+                    // pass
+                  }
+                  break;
+              }
+            }
+          } while (true);
+        }
+      }
+    }
+};