Fix NPath computation

aibolit/metrics/npath/main.py

@@ -104,57 +104,65 @@ def _node_npath(self, node: ASTNode) -> int:
             return self._sequence_npath(node)
 
         def default_handler(node: ASTNode) -> int:
-            return self._sequence_npath(node.children)
+            return self._composite_npath(node)
 
         dispatcher = {
             ASTNodeType.IF_STATEMENT: self._if_npath,
             ASTNodeType.SWITCH_STATEMENT: self._switch_npath,
             ASTNodeType.FOR_STATEMENT: self._for_loop_npath,
             ASTNodeType.WHILE_STATEMENT: self._while_loop_npath,
             ASTNodeType.BINARY_OPERATION: self._binary_expression_npath,
+            ASTNodeType.EXPRESSION: self._expression_npath,
         }
 
         handler = dispatcher.get(node.node_type, default_handler)
         return handler(node)
 
+    def _composite_npath(self, node: ASTNode) -> int:
+        return self._sequence_npath(node.children)
+
     def _sequence_npath(self, nodes: Iterable[ASTNode]) -> int:
-        return math.prod((self._node_npath(child) for child in nodes))
+        return max(1, math.prod(self._node_npath(child) for child in nodes))
 
     def _if_npath(self, node: ASTNode) -> int:
-        condition_npath = self._node_npath(node.condition)
+        condition_npath = self._expression_npath(node.condition)
         then_npath = self._node_npath(node.then_statement)
         else_npath = (
             self._node_npath(node.else_statement)
             if node.else_statement is not None
             else 1
         )
-        return condition_npath * then_npath + else_npath
+        return condition_npath + then_npath + else_npath
 
     def _switch_npath(self, node: ASTNode) -> int:
-        def _group_npath(case_group: ASTNode) -> int:
-            return math.prod((self._node_npath(case) for case in case_group.children))
-
-        case_paths = sum(_group_npath(case_group) for case_group in node.cases)
-        return self._node_npath(node.expression) * max(case_paths, 1)
+        npath = self._expression_npath(node.expression)
+        has_case = False
+        has_default = False
+        for case_group in node.cases:
+            if len(case_group.case):
+                has_case = True
+            else:
+                has_default = True
+            null_statements = max(0, len(case_group.case) - 1)
+            npath += self._node_npath(case_group.statements) + null_statements
+        return npath + int(not has_case) + int(not has_default)
 
     def _for_loop_npath(self, node: ASTNode) -> int:
-        condition_npath = self._condition_npath(node.control)
+        control_npath = self._expression_npath(node.control)
         body_npath = self._node_npath(node.body)
-        return condition_npath + body_npath
+        return control_npath + body_npath + 1
 
     def _while_loop_npath(self, node: ASTNode) -> int:
-        condition_npath = self._condition_npath(node)
+        condition_npath = self._expression_npath(node.condition)
         body_npath = self._node_npath(node.body)
-        return condition_npath + body_npath
+        return condition_npath + body_npath + 1
 
-    def _condition_npath(self, node: ASTNode) -> int:
-        if hasattr(node, 'condition') and node.condition:
-            return max(1, self._node_npath(node.condition))
-        return 1
+    def _expression_npath(self, node: ASTNode) -> int:
+        if node.node_type == ASTNodeType.BINARY_OPERATION:
+            return self._binary_expression_npath(node)
+        return sum(self._expression_npath(child) for child in node.children)
 
     def _binary_expression_npath(self, node: ASTNode) -> int:
-        if node.operator not in {'&&', '||'}:
-            return 1
-        left_npath = self._node_npath(node.operandl)
-        right_npath = self._node_npath(node.operandr)
-        return left_npath + right_npath
+        left_npath = self._expression_npath(node.operandl)
+        right_npath = self._expression_npath(node.operandr)
+        return left_npath + right_npath + (node.operator in ('&&', '||'))

test/metrics/npath/test_all_types.py

@@ -247,7 +247,8 @@ class Test {
             }
             ''',
         ).strip()
-        assert self._value(content) == 1
+        # NPath = 2 = 1 (for an empty CASE) + 1 (for an empty DEFAULT)
+        assert self._value(content) == 2
 
     def test_switch_simple_without_default(self):
         content = dedent(
@@ -262,7 +263,8 @@ class Test {
             }
             ''',
         ).strip()
-        assert self._value(content) == 2
+        # NPath = 3 = 2 (for 2 CASEs) + 1 (for an empty DEFAULT)
+        assert self._value(content) == 3
 
     def test_switch_with_fallthrough(self):
         content = dedent(
@@ -280,7 +282,8 @@ class Test {
             }
             ''',
         )
-        assert self._value(content) == 4
+        # NPath = 5 = 1 for each CASE and DEFAULT
+        assert self._value(content) == 5
 
     def test_nested_switch_statements(self) -> None:
         content = dedent(
@@ -539,52 +542,10 @@ def test_nested_while_loops(self) -> None:
         ''').strip()
         assert self._value(content) == 3
 
-    @pytest.mark.xfail(
-        strict=True,
-        reason='Incorrect implementation. Most likely need to review if statement',
-    )
     def test_complicated(self) -> None:
-        # @todo #852:60min Fix MvnFreeNPathMetric for moderate NPath complexity case
-        #  It is necessary to fix implementation of MvnFreeNPathMetric
-        #  so that the test on a moderate NPath complexity case passes.
-        #  Once fixed, remove `pytest.mark.xfail` decorator.
-        #
-        #  It is most likely that the implementation of if statements is wrong.
-        #  Refer to https://checkstyle.org/checks/metrics/npathcomplexity.html,
-        #  where it is stated that complexity for the `if` statement is a sum
-        #  of NPath complexities for the condition, then and else parts.
-        #  On the other hand when following this calculation, the simple if-else statement
-        #  would have complexity of 3 rather than 2.
         assert self._value_from_filepath(self._filepath('Complicated.java')) == 12
 
-    @pytest.mark.xfail(
-        strict=True,
-        reason='Incorrect implementation. Most likely need to review if/switch statements',
-    )
     def test_even_more_complicated(self) -> None:
-        # @todo #852:60min Fix MvnFreeNPathMetric for high NPath complexity case
-        #  It is necessary to fix implementation of MvnFreeNPathMetric,
-        #  so that the test on a very high NPath complexity case passes.
-        #  Once fixed, remove `pytest.mark.xfail` decorator.
-        #
-        #  Although the comment in Foo.java states that the expected value is 200,
-        #  the actual value, verified against `checkstyle-10.26.1-all.jar` is 288.
-        #
-        #  $ cat config.xml
-        #  <?xml version="1.0"?>
-        #  <!DOCTYPE module PUBLIC
-        #            "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
-        #            "https://checkstyle.org/dtds/configuration_1_3.dtd">
-        #  <module name="Checker">
-        #    <module name="TreeWalker">
-        #      <module name="NPathComplexity">
-        #        <property name="max" value="1"/>
-        #      </module>
-        #    </module>
-        #  </module>
-        #
-        #  $ java -jar checkstyle-10.26.1-all.jar -c config.xml test/metrics/npath/Foo.java
-        #  [ERROR] ... NPath Complexity is 288 (max allowed is 1). [NPathComplexity]
         assert self._value_from_filepath(self._filepath('Foo.java')) == 288
 
     def _filepath(self, basename: str) -> pathlib.Path: