Repository: terryyin/lizard

Issue:
Detection of Deeply Nested Control Structures
This metric may not apply to the whole function,
but the maximum 'nestedness' (nesting for-loops, if-statements, etc.)
may be an interesting metric to detect code smell.

It closely relates to indentation. 
Got this from the Linux kernel coding style:
>The answer to that is that if you need
more than 3 levels of indentation, you're screwed anyway, and should fix
your program.

Discussion:
rakhimov: Hi @terryyin,
do not yet merge this PR.
I sense there are bugs in implementation of C++ ``current_nesting_level``.
The initial code works with Python,
for which the nesting level metric is producing what is expected.
However, the C++ ``current_nesting_level`` doesn't seem to be fully conformant.

Your suggestions are welcome.

Patch:
diff --git a/lizard.py b/lizard.py
index 4d21e8a..cac8020 100755
--- a/lizard.py
+++ b/lizard.py
@@ -316,7 +316,7 @@ class NestingStack(object):
         self.pending_function = None
         self.nesting_stack.append(Namespace(token))
 
-    def start_new_funciton_nesting(self, function):
+    def start_new_function_nesting(self, function):
         self.pending_function = function
 
     def _create_nesting(self):
@@ -386,7 +386,7 @@ class FileInfoBuilder(object):
             self.fileinfo.filename,
             self.current_line)
         self.current_function.top_nesting_level = self.current_nesting_level
-        self.start_new_funciton_nesting(self.current_function)
+        self.start_new_function_nesting(self.current_function)
 
     def add_condition(self, inc=1):
         self.current_function.cyclomatic_complexity += inc
diff --git a/lizard_ext/lizardns.py b/lizard_ext/lizardns.py
index e057e73..4ee09bf 100644
--- a/lizard_ext/lizardns.py
+++ b/lizard_ext/lizardns.py
@@ -1,13 +1,16 @@
 """
 This extension counts nested control structures within a function.
-The extension is implemented with C++ in mind.
+
+The extension is implemented with C++ and Python in mind,
+but it is expected to work with other languages supported by Lizard
+with its language reader implementing 'nesting_level' metric for tokens.
 
 The code borrows heavily from implementation of Nesting Depth extension
 originally written by Mehrdad Meh and Terry Yin.
 """
 
-from lizard import FileInfoBuilder, FunctionInfo
-from lizard_ext.lizardnd import patch, patch_append_method
+from lizard import FunctionInfo
+from lizard_ext.lizardnd import patch_append_method
 
 
 DEFAULT_NS_THRESHOLD = 3
@@ -32,106 +35,90 @@ class LizardExtension(object):  # pylint: disable=R0903
     def __call__(self, tokens, reader):
         """The intent of the code is to detect control structures as entities.
 
-        The complexity arises from tracking of
-        control structures without brackets.
-        The termination of such control structures in C-like languages
-        is the next statement or control structure with a compound statement.
-
-        Moreover, control structures with two or more tokens complicates
-        the proper counting, for example, 'else if'.
+        The implementation relies on nesting level metric for tokens
+        provided by language readers.
+        If the following contract for the nesting level metric does not hold,
+        this implementation of nested structure counting is invalid.
 
-        In Python with meaningful indentation,
-        tracking the indentation levels becomes crucial
-        to identify boundaries of the structures.
-        The following code is not designed for Python.
-        """
-        structures = set(['if', 'else', 'foreach', 'for', 'while', 'do',
-                          'try', 'catch', 'switch'])
+        If a control structure has started its block (eg. '{'),
+        and its level is **less** than the next structure,
+        the next structure is nested.
 
-        structure_indicator = "{"
-        structure_end = "}"
-        indent_indicator = ";"
-
-        for token in tokens:
-            if reader.context.is_within_structure():
-                if token == "(":
-                    reader.context.add_parentheses(1)
-                elif token == ")":
-                    reader.context.add_parentheses(-1)
+        If a control structure has *not* started its block,
+        and its level is **no more** than the next structure,
+        the next structure is nested (compound statement).
 
-            if not reader.context.is_within_parentheses():
-                if token in structures:
-                    reader.context.add_nested_structure(token)
+        If a control structure level is **higher** than the next structure,
+        it is considered closed.
 
-                elif token == structure_indicator:
-                    reader.context.add_brace()
-
-                elif token == structure_end:
-                    reader.context.pop_brace()
-                    reader.context.pop_nested_structure()
-
-                elif token == indent_indicator:
-                    reader.context.pop_nested_structure()
-
-            yield token
+        If a control structure has started its block,
+        and its level is **equal** to the next structure,
+        it is considered closed.
 
-
-# TODO: Some weird false positive from pylint. # pylint: disable=fixme
-# pylint: disable=E1101
-class NSFileInfoAddition(FileInfoBuilder):
-
-    def add_nested_structure(self, token):
-        """Conditionally adds nested structures."""
-        # Handle compound else-if.
-        if token == "if" and self.current_function.structure_stack:
-            prev_token, br_state = self.current_function.structure_stack[-1]
-            if (prev_token == "else" and
-                    br_state == self.current_function.brace_count):
+        The level of any non-structure tokens is treated
+        with the same logic as for the next structures
+        for control block **starting** and **closing** purposes.
+        """
+        # TODO: Delegate this to language readers  # pylint: disable=fixme
+        structures = set(['if', 'else', 'elif', 'for', 'foreach', 'while', 'do',
+                          'try', 'catch', 'switch', 'finally', 'except',
+                          'with'])
+
+        cur_level = 0
+        start_structure = [False]  # Just to make it mutable.
+        structure_stack = []  # [(token, ns_level)]
+
+        def add_nested_structure(token):
+            """Conditionally adds nested structures."""
+            if structure_stack:
+                prev_token, ns_level = structure_stack[-1]
+                if cur_level == ns_level:
+                    if (token == "if" and prev_token == "else" and
+                            not start_structure[0]):
+                        return  # Compound 'else if' in C-like languages.
+                    if start_structure[0]:
+                        structure_stack.pop()
+                elif cur_level < ns_level:
+                    while structure_stack and ns_level >= cur_level:
+                        _, ns_level = structure_stack.pop()
+
+            structure_stack.append((token, cur_level))
+            start_structure[0] = False  # Starts on the next level with body.
+
+            ns_cur = len(structure_stack)
+            if reader.context.current_function.max_nested_structures < ns_cur:
+                reader.context.current_function.max_nested_structures = ns_cur
+
+        def pop_nested_structure():
+            """Conditionally pops the nested structures if levels match."""
+            if not structure_stack:
                 return
 
-        self.current_function.structure_stack.append(
-            (token, self.current_function.brace_count))
-
-        ns_cur = len(self.current_function.structure_stack)
-        if self.current_function.max_nested_structures < ns_cur:
-            self.current_function.max_nested_structures = ns_cur
+            _, ns_level = structure_stack[-1]
 
-    def pop_nested_structure(self):
-        """Conditionally pops the structure count if braces match."""
-        if not self.current_function.structure_stack:
-            return
+            if cur_level > ns_level:
+                start_structure[0] = True
 
-        _, br_state = self.current_function.structure_stack[-1]
-        if br_state == self.current_function.brace_count:
-            self.current_function.structure_stack.pop()
+            elif cur_level < ns_level:
+                while structure_stack and ns_level >= cur_level:
+                    _, ns_level = structure_stack.pop()
+                start_structure[0] = bool(structure_stack)
 
-    def add_brace(self):
-        self.current_function.brace_count += 1
+            elif start_structure[0]:
+                structure_stack.pop()
 
-    def pop_brace(self):
-        # pylint: disable=fixme
-        # TODO: For some reason, brace count goes negative.
-        # assert self.current_function.brace_count > 0
-        self.current_function.brace_count -= 1
-
-    def add_parentheses(self, inc):
-        """Dual purpose parentheses manipulator."""
-        self.current_function.paren_count += inc
-
-    def is_within_parentheses(self):
-        assert self.current_function.paren_count >= 0
-        return self.current_function.paren_count != 0
+        for token in tokens:
+            cur_level = reader.context.current_nesting_level
+            if token in structures:
+                add_nested_structure(token)
+            else:
+                pop_nested_structure()
 
-    def is_within_structure(self):
-        return bool(self.current_function.structure_stack)
+            yield token
 
 
 def _init_nested_structure_data(self, *_):
     self.max_nested_structures = 0
-    self.brace_count = 0
-    self.paren_count = 0
-    self.structure_stack = []
 
 
-patch(NSFileInfoAddition, FileInfoBuilder)
 patch_append_method(_init_nested_structure_data, FunctionInfo, "__init__")


Test patch:
diff --git a/test/testNestedStructures.py b/test/testNestedStructures.py
old mode 100755
new mode 100644
index 7eee514..1a2d826
--- a/test/testNestedStructures.py
+++ b/test/testNestedStructures.py
@@ -1,5 +1,7 @@
 import unittest
-from .testHelpers import get_cpp_function_list_with_extnesion
+
+from .testHelpers import get_cpp_function_list_with_extnesion, \
+    get_python_function_list_with_extnesion
 from lizard_ext.lizardns import LizardExtension as NestedStructure
 
 
@@ -7,6 +9,10 @@ def process_cpp(source):
     return get_cpp_function_list_with_extnesion(source, NestedStructure())
 
 
+def process_python(source):
+    return get_python_function_list_with_extnesion(source, NestedStructure())
+
+
 class TestCppNestedStructures(unittest.TestCase):
 
     def test_no_structures(self):
@@ -209,3 +215,122 @@ class TestCppNestedStructures(unittest.TestCase):
         }
         """)
         self.assertEqual(3, result[0].max_nested_structures)
+
+
+class TestPythonNestedStructures(unittest.TestCase):
+
+    def test_no_structures(self):
+        result = process_python("def fun():\n pass")
+        self.assertEqual(0, result[0].max_nested_structures)
+
+    def test_if_structure(self):
+        result = process_python("def fun():\n if a:\n  return")
+        self.assertEqual(1, result[0].max_nested_structures)
+
+    def test_for_structure(self):
+        result = process_python("def fun():\n for a in b:\n  foo()")
+        self.assertEqual(1, result[0].max_nested_structures)
+
+    def test_condition_in_if_structure(self):
+        result = process_python("def fun():\n if a and b:\n  return")
+        self.assertEqual(1, result[0].max_nested_structures)
+
+    def test_elif(self):
+        result = process_python("""
+        def c():
+          if a:
+            baz()
+          elif c:
+            foo()
+        """)
+        self.assertEqual(1, result[0].max_nested_structures)
+
+    def test_nested_if_structures(self):
+        result = process_python("""
+        def c():
+          if a:
+            if b:
+              baz()
+          else:
+            foo()
+        """)
+        self.assertEqual(2, result[0].max_nested_structures)
+
+    def test_equal_metric_structures(self):
+        result = process_python("""
+        def c():
+          if a:
+            if b:
+              baz()
+          else:
+            foo()
+
+          for a in b:
+            if c:
+              bar()
+        """)
+        self.assertEqual(2, result[0].max_nested_structures)
+
+    def test_while(self):
+        result = process_python("""
+        def c():
+          while a:
+            baz()
+        """)
+        self.assertEqual(1, result[0].max_nested_structures)
+
+    def test_try_catch(self):
+        result = process_python("""
+        def c():
+          try:
+            f.open()
+          catch Exception as err:
+            print(err)
+          finally:
+            f.close()
+        """)
+        self.assertEqual(1, result[0].max_nested_structures)
+
+    def test_two_functions(self):
+        result = process_python("""
+        def c():
+          try:
+            if a:
+              foo()
+          catch Exception as err:
+            print(err)
+
+        def d():
+          for a in b:
+            for x in y:
+              if i:
+                return j
+        """)
+        self.assertEqual(2, result[0].max_nested_structures)
+        self.assertEqual(3, result[1].max_nested_structures)
+
+    def test_nested_functions(self):
+        result = process_python("""
+        def c():
+            def d():
+                for a in b:
+                    for x in y:
+                        if i:
+                            return j
+            try:
+                if a:
+                    foo()
+            catch Exception as err:
+                print(err)
+
+        """)
+        self.assertEqual(3, result[0].max_nested_structures)
+        self.assertEqual(2, result[1].max_nested_structures)
+
+    def test_with_structure(self):
+        result = process_python("""
+        def c():
+            with open(f) as input_file:
+                foo(f)
+        """)
+        self.assertEqual(1, result[0].max_nested_structures)