From 4eb68bcbad612434ef7501bd0c72f2f3ee7ceacb Mon Sep 17 00:00:00 2001 From: Yaroslav Kishchenko Date: Fri, 17 Jul 2020 11:23:12 +0300 Subject: [PATCH 1/5] Prettier ASTNode string representation. --- aibolit/ast_framework/ast_node.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/aibolit/ast_framework/ast_node.py b/aibolit/ast_framework/ast_node.py index 010c9911..079800de 100644 --- a/aibolit/ast_framework/ast_node.py +++ b/aibolit/ast_framework/ast_node.py @@ -64,7 +64,10 @@ def __str__(self) -> str: text_representation = f'node index: {self._node_index}' node_type = self.__getattr__('node_type') for attribute_name in sorted(common_attributes | attributes_by_node_type[node_type]): - text_representation += f'\n{attribute_name}: {self.__getattr__(attribute_name)}' + attribute_value = self.__getattr__(attribute_name) + attribute_representation = \ + repr(attribute_value) if isinstance(attribute_value, ASTNode) else str(attribute_value) + text_representation += f'\n{attribute_name}: {attribute_representation}' return text_representation From 25a721559acaef33245aa53c2d51808b4023a126 Mon Sep 17 00:00:00 2001 From: Yaroslav Kishchenko Date: Fri, 17 Jul 2020 11:23:41 +0300 Subject: [PATCH 2/5] Add eq and hash methods to ASTNode. --- aibolit/ast_framework/ast_node.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/aibolit/ast_framework/ast_node.py b/aibolit/ast_framework/ast_node.py index 079800de..94e0c9fb 100644 --- a/aibolit/ast_framework/ast_node.py +++ b/aibolit/ast_framework/ast_node.py @@ -74,5 +74,14 @@ def __str__(self) -> str: def __repr__(self) -> str: return f'' + def __eq__(self, other: object) -> bool: + if not isinstance(other, ASTNode): + raise NotImplementedError(f'ASTNode support comparission only with themselves, \ + but {type(other)} was provided.') + return self._graph == other._graph and self._node_index == other._node_index + + def __hash__(self): + return hash(self._node_index) + # names of methods and properties, which is not generated dynamically _public_fixed_interface = ['children', 'node_index', 'subtree'] From 6566cee4b11eab0a9d855f96f9d1bcdf76803c81 Mon Sep 17 00:00:00 2001 From: Yaroslav Kishchenko Date: Fri, 17 Jul 2020 11:24:48 +0300 Subject: [PATCH 3/5] Add ability to specify several types in `get_proxy_nodes` method. --- aibolit/ast_framework/ast.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/aibolit/ast_framework/ast.py b/aibolit/ast_framework/ast.py index 0c6d301f..72ca8ffe 100644 --- a/aibolit/ast_framework/ast.py +++ b/aibolit/ast_framework/ast.py @@ -162,9 +162,9 @@ def get_nodes(self, type: Union[ASTNodeType, None] = None) -> Iterator[int]: if type is None or self.tree.nodes[node]['node_type'] == type: yield node - def get_proxy_nodes(self, type: ASTNodeType) -> Iterator[ASTNode]: + def get_proxy_nodes(self, *types: ASTNodeType) -> Iterator[ASTNode]: for node in self.tree.nodes: - if self.tree.nodes[node]['node_type'] == type: + if len(types) == 0 or self.tree.nodes[node]['node_type'] in types: yield ASTNode(self.tree, node) @deprecated(reason='Use ASTNode functionality instead.') From 7f63e9e133e045a9f929513c83dd7b71a62d3c82 Mon Sep 17 00:00:00 2001 From: Yaroslav Kishchenko Date: Fri, 17 Jul 2020 11:33:26 +0300 Subject: [PATCH 4/5] NCSS refactored. Tests are corrected. Now metrics do not count "try" statements, but count "else" statements. --- aibolit/metrics/ncss/ncss.py | 136 +++++++++++++++++--------- test/metrics/ncss/BasicExample.java | 40 ++++---- test/metrics/ncss/Empty.java | 2 +- test/metrics/ncss/Simple.java | 12 ++- test/metrics/ncss/SimpleExample.java | 48 ++++----- test/metrics/ncss/SimpleExample2.java | 48 ++++----- test/metrics/ncss/test_all_types.py | 4 +- 7 files changed, 169 insertions(+), 121 deletions(-) diff --git a/aibolit/metrics/ncss/ncss.py b/aibolit/metrics/ncss/ncss.py index f35b073f..77559e13 100644 --- a/aibolit/metrics/ncss/ncss.py +++ b/aibolit/metrics/ncss/ncss.py @@ -20,59 +20,99 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE # SOFTWARE. -from networkx import dfs_labeled_edges # type: ignore - +from aibolit.ast_framework import AST, ASTNode, ASTNodeType from aibolit.utils.ast_builder import build_ast -from aibolit.ast_framework import AST, ASTNodeType class NCSSMetric(): - def __init__(self): - pass - - def value(self, filename: str): - set_of_the_types = {ASTNodeType.ANNOTATION_DECLARATION, - ASTNodeType.CLASS_DECLARATION, - ASTNodeType.CONSTANT_DECLARATION, - ASTNodeType.CONSTRUCTOR_DECLARATION, - ASTNodeType.DECLARATION, - ASTNodeType.ENUM_CONSTANT_DECLARATION, - ASTNodeType.ENUM_DECLARATION, - ASTNodeType.FIELD_DECLARATION, - ASTNodeType.INTERFACE_DECLARATION, - ASTNodeType.LOCAL_VARIABLE_DECLARATION, - ASTNodeType.METHOD_DECLARATION, - ASTNodeType.TYPE_DECLARATION, - ASTNodeType.VARIABLE_DECLARATION, - ASTNodeType.CATCH_CLAUSE, - ASTNodeType.ASSERT_STATEMENT, - ASTNodeType.BREAK_STATEMENT, - ASTNodeType.CONTINUE_STATEMENT, - ASTNodeType.DO_STATEMENT, - ASTNodeType.FOR_STATEMENT, - ASTNodeType.IF_STATEMENT, - ASTNodeType.RETURN_STATEMENT, - ASTNodeType.STATEMENT, - ASTNodeType.STATEMENT_EXPRESSION, - ASTNodeType.SWITCH_STATEMENT, - ASTNodeType.SWITCH_STATEMENT_CASE, - ASTNodeType.SYNCHRONIZED_STATEMENT, - ASTNodeType.THROW_STATEMENT, - ASTNodeType.TRY_STATEMENT, - ASTNodeType.WHILE_STATEMENT} - - if len(filename) == 0: - raise ValueError('Empty file for analysis') + ''' + NCSS metric counts non-commenting source statements. + It counts keywords, declarations and statement expressions. + Following description was used as a reference: + https://pmd.github.io/latest/pmd_java_metrics_index.html#non-commenting-source-statements-ncss + ''' - tree = AST.build_from_javalang(build_ast(filename)) + def value(self, filename: str) -> int: metric = 0 - for _, destination, edge_type in dfs_labeled_edges(tree.tree, tree.root): - if edge_type == 'forward': - node_type = tree.get_type(destination) - if node_type in set_of_the_types: - metric += 1 - if node_type == ASTNodeType.FOR_CONTROL: - metric -= len(list(tree.children_with_type(destination, ASTNodeType.VARIABLE_DECLARATION))) - metric -= len(list(tree.children_with_type(destination, ASTNodeType.LOCAL_VARIABLE_DECLARATION))) + ast = AST.build_from_javalang(build_ast(filename)) + for node in ast.get_proxy_nodes(*NCSSMetric._keyword_node_types, + *NCSSMetric._declarations_node_types, + *NCSSMetric._misc_node_types): + metric += 1 + if node.node_type == ASTNodeType.IF_STATEMENT: + metric += self._count_else_statements(node) + + for try_statement in ast.get_proxy_nodes(ASTNodeType.TRY_STATEMENT): + if self._has_finally_block(try_statement): + metric += 1 return metric + + def _count_else_statements(self, if_statement: ASTNode) -> int: + assert(if_statement.node_type == ASTNodeType.IF_STATEMENT) + else_statements_qty = 0 + # elif_statement might be: + # - None, if there is no "else" branch + # - BLOCK_STATEMENT, if there is "else" branch + # - IF_STATEMENT, if there is "elif" branch + elif_statement = if_statement.else_statement + + # iterating over sequence of "elif" branches + while elif_statement is not None and elif_statement.node_type == ASTNodeType.IF_STATEMENT: + else_statements_qty += 1 + elif_statement = elif_statement.else_statement + + # check for the possible "else" branch in the end of "elif" sequence + if elif_statement is not None: + else_statements_qty += 1 + + return else_statements_qty + + def _has_finally_block(self, try_statement: ASTNode) -> bool: + assert(try_statement.node_type == ASTNodeType.TRY_STATEMENT) + return try_statement.finally_block is not None + + # Two keywords "else" and "finally" are not represented by any nodes + # and have to be extracted manually from fields + # of IF_STATEMENT and TRY_STATEMENT respectively + _keyword_node_types = { + ASTNodeType.ASSERT_STATEMENT, + ASTNodeType.BREAK_STATEMENT, + ASTNodeType.CATCH_CLAUSE, + ASTNodeType.CONTINUE_STATEMENT, + ASTNodeType.DO_STATEMENT, + ASTNodeType.FOR_STATEMENT, + ASTNodeType.IF_STATEMENT, + ASTNodeType.RETURN_STATEMENT, + ASTNodeType.SWITCH_STATEMENT_CASE, + ASTNodeType.SWITCH_STATEMENT, + ASTNodeType.SYNCHRONIZED_STATEMENT, + ASTNodeType.THROW_STATEMENT, + ASTNodeType.WHILE_STATEMENT, + } + + # There are two type of declarations: type declarations (annotation, class etc.) + # and class parts declarations (fields, methods etc.) + # Package declarations are out of our interest, because it makes harder to compare + # nested and outer classes. + _declarations_node_types = { + ASTNodeType.ANNOTATION_DECLARATION, + ASTNodeType.CLASS_DECLARATION, + ASTNodeType.CONSTANT_DECLARATION, + ASTNodeType.CONSTRUCTOR_DECLARATION, + ASTNodeType.ENUM_CONSTANT_DECLARATION, + ASTNodeType.ENUM_DECLARATION, + ASTNodeType.FIELD_DECLARATION, + ASTNodeType.INTERFACE_DECLARATION, + ASTNodeType.METHOD_DECLARATION, + ASTNodeType.TYPE_DECLARATION, + } + + _misc_node_types = { + # Represent declaration of all local variables + # *excluding* declarations inside "for" loop control. + # Those declarations are represented by "VARIABLE_DECLARATION" nodes. + ASTNodeType.LOCAL_VARIABLE_DECLARATION, + # Statement expressions also includes assignments + ASTNodeType.STATEMENT_EXPRESSION, + } diff --git a/test/metrics/ncss/BasicExample.java b/test/metrics/ncss/BasicExample.java index 20a21f58..7784c44c 100644 --- a/test/metrics/ncss/BasicExample.java +++ b/test/metrics/ncss/BasicExample.java @@ -1,25 +1,27 @@ -import java.util.Collections; // +0 -import java.io.IOException; // +0 +// Total NCSS = 12 -class Foo { // +1, total Ncss = 12 - - public void bigMethod() // +1 - throws IOException { - int x = 0, y = 2; // +1 - boolean a = false, b = true; // +1 - - if (a || b) { // +1 - try { // +1 - do { // +1 - x += 2; // +1 +import java.util.Collections; +import java.io.IOException; + +class Foo { // +1 + + public void bigMethod() // +1 + throws IOException { + int x = 0, y = 2; // +1 + boolean a = false, b = true; // +1 + + if (a || b) { // +1 + try { + do { // +1 + x += 2; // +1 } while (x < 12); - - System.exit(0); // +1 - } catch (IOException ioe) { // +1 + + System.exit(0); // +1 + } catch (IOException ioe) { // +1 throw new PatheticFailException(ioe); // +1 } - } else { - assert false; // +1 + } else { // +1 + assert false; // +1 } - } + } } diff --git a/test/metrics/ncss/Empty.java b/test/metrics/ncss/Empty.java index 8b137891..14a59834 100644 --- a/test/metrics/ncss/Empty.java +++ b/test/metrics/ncss/Empty.java @@ -1 +1 @@ - +// Total NCSS = 0 diff --git a/test/metrics/ncss/Simple.java b/test/metrics/ncss/Simple.java index f470db63..66895192 100644 --- a/test/metrics/ncss/Simple.java +++ b/test/metrics/ncss/Simple.java @@ -1,9 +1,11 @@ -import java.util.Collections; -import java.io.IOException; +// Total NCSS = 2 -class Foo { - - public void bigMethod() { +import java.util.Collections; +import java.io.IOException; + +class Foo { // +1 + + public void bigMethod() { // +1 } } diff --git a/test/metrics/ncss/SimpleExample.java b/test/metrics/ncss/SimpleExample.java index 6992fabb..197467ad 100644 --- a/test/metrics/ncss/SimpleExample.java +++ b/test/metrics/ncss/SimpleExample.java @@ -1,29 +1,31 @@ -import java.util.Collections; // +0 -import java.io.IOException; // +0 +// Total NCSS = 17 -class Foo { // +1, total Ncss = 16 - - public void bigMethod() // +1 - throws IOException { - int x, y = 2; // +1 - boolean a, b; // +1 - a = false; // +1 - b = true; // +1 - x = 0; // +1 - if (a || b) { // +1 - try { // +1 - do { // +1 - x += 2; // +1 +import java.util.Collections; +import java.io.IOException; + +class Foo { // +1 + + public void bigMethod() // +1 + throws IOException { + int x, y = 2; // +1 + boolean a, b; // +1 + a = false; // +1 + b = true; // +1 + x = 0; // +1 + if (a || b) { // +1 + try { + do { // +1 + x += 2; // +1 } while (x < 12); - - System.exit(0); // +1 - } catch (IOException ioe) { // +1 + + System.exit(0); // +1 + } catch (IOException ioe) { // +1 throw new PatheticFailException(ioe); // +1 - } finally { // +1 - System.out.println("Finally Block"); + } finally { // +1 + System.out.println("Finally Block"); // +1 } - } else { - assert false; // +1 + } else { // +1 + assert false; // +1 } - } + } } diff --git a/test/metrics/ncss/SimpleExample2.java b/test/metrics/ncss/SimpleExample2.java index 33acf583..e7c09704 100644 --- a/test/metrics/ncss/SimpleExample2.java +++ b/test/metrics/ncss/SimpleExample2.java @@ -1,29 +1,31 @@ -import java.util.Collections; // +0 -import java.io.IOException; // +0 +// Total NCSS = 18 -class Foo { // +1, total Ncss = 16 - - public void bigMethod() // +1 - throws IOException { - int x, y = 2; // +1 - boolean a, b; // +1 - a = false; // +1 - b = true; // +1 - x = 0; // +1 - if (a || b) { // +1 - try { // +1 - for(int i = 0, j = 10; i < j; i++){ // +1 - x += 2; // +1 +import java.util.Collections; +import java.io.IOException; + +class Foo { // +1 + public void bigMethod() // +1 + throws IOException { + int x, y = 2; // +1 + boolean a, b; // +1 + a = false; // +1 + b = true; // +1 + x = 0; // +1 + if (a || b) { // +1 + try { + int i, j; // +1 + for(i = 0, j = 10; i < j; i++) { // +1 + x += 2; // +1 } - - System.exit(0); // +1 - } catch (IOException ioe) { // +1 + + System.exit(0); // +1 + } catch (IOException ioe) { // +1 throw new PatheticFailException(ioe); // +1 - } finally { // +1 - System.out.println("Finally Block"); + } finally { // +1 + System.out.println("Finally Block"); // +1 } - } else { - assert false; // +1 + } else { // +1 + assert false; // +1 } - } + } } diff --git a/test/metrics/ncss/test_all_types.py b/test/metrics/ncss/test_all_types.py index 4df36b1a..ce5cdebe 100644 --- a/test/metrics/ncss/test_all_types.py +++ b/test/metrics/ncss/test_all_types.py @@ -47,10 +47,10 @@ def testSimpleExample(self): file = 'test/metrics/ncss/SimpleExample.java' metric = NCSSMetric() res = metric.value(file) - self.assertEqual(res, 16) + self.assertEqual(res, 17) def testSimpleExample2(self): file = 'test/metrics/ncss/SimpleExample2.java' metric = NCSSMetric() res = metric.value(file) - self.assertEqual(res, 16) + self.assertEqual(res, 18) From 2984feaed8568e9e510fe80cd151efd254c0a97f Mon Sep 17 00:00:00 2001 From: Yaroslav Kishchenko Date: Fri, 17 Jul 2020 12:11:03 +0300 Subject: [PATCH 5/5] Correct else branches handling. Adding test for else branches and finally block. --- aibolit/metrics/ncss/ncss.py | 30 +++++++------------ test/metrics/ncss/ChainedIfElse.java | 17 +++++++++++ .../ncss/ChainedIfElseWithTrailingElse.java | 18 +++++++++++ test/metrics/ncss/FinallyBlock.java | 16 ++++++++++ test/metrics/ncss/test_all_types.py | 18 +++++++++++ 5 files changed, 79 insertions(+), 20 deletions(-) create mode 100644 test/metrics/ncss/ChainedIfElse.java create mode 100644 test/metrics/ncss/ChainedIfElseWithTrailingElse.java create mode 100644 test/metrics/ncss/FinallyBlock.java diff --git a/aibolit/metrics/ncss/ncss.py b/aibolit/metrics/ncss/ncss.py index 77559e13..ea8a335e 100644 --- a/aibolit/metrics/ncss/ncss.py +++ b/aibolit/metrics/ncss/ncss.py @@ -39,8 +39,8 @@ def value(self, filename: str) -> int: *NCSSMetric._declarations_node_types, *NCSSMetric._misc_node_types): metric += 1 - if node.node_type == ASTNodeType.IF_STATEMENT: - metric += self._count_else_statements(node) + if node.node_type == ASTNodeType.IF_STATEMENT and self._has_pure_else_statements(node): + metric += 1 for try_statement in ast.get_proxy_nodes(ASTNodeType.TRY_STATEMENT): if self._has_finally_block(try_statement): @@ -48,25 +48,15 @@ def value(self, filename: str) -> int: return metric - def _count_else_statements(self, if_statement: ASTNode) -> int: + def _has_pure_else_statements(self, if_statement: ASTNode) -> bool: + ''' + Checks is there else branch. + If else branch appeared to be "else if" construction (not pure "else"), + returns False. + ''' assert(if_statement.node_type == ASTNodeType.IF_STATEMENT) - else_statements_qty = 0 - # elif_statement might be: - # - None, if there is no "else" branch - # - BLOCK_STATEMENT, if there is "else" branch - # - IF_STATEMENT, if there is "elif" branch - elif_statement = if_statement.else_statement - - # iterating over sequence of "elif" branches - while elif_statement is not None and elif_statement.node_type == ASTNodeType.IF_STATEMENT: - else_statements_qty += 1 - elif_statement = elif_statement.else_statement - - # check for the possible "else" branch in the end of "elif" sequence - if elif_statement is not None: - else_statements_qty += 1 - - return else_statements_qty + return if_statement.else_statement is not None and \ + if_statement.else_statement.node_type != ASTNodeType.IF_STATEMENT def _has_finally_block(self, try_statement: ASTNode) -> bool: assert(try_statement.node_type == ASTNodeType.TRY_STATEMENT) diff --git a/test/metrics/ncss/ChainedIfElse.java b/test/metrics/ncss/ChainedIfElse.java new file mode 100644 index 00000000..5f06b0a3 --- /dev/null +++ b/test/metrics/ncss/ChainedIfElse.java @@ -0,0 +1,17 @@ +// Total NCSS = 11 + +class ChainedIfElse { // +1 + public int chainedIfElseMethod(int x) { // +1 + if(x > 10) { // +1 + return 100; // +1 + } else if(x > 5) { // +1 + return 5; // +1 + } else if (x > 0) { // +1 + return 1; // +1 + } else if (x == 0) { // +1 + return 0; // +1 + } + + return -1; // +1 + } +} \ No newline at end of file diff --git a/test/metrics/ncss/ChainedIfElseWithTrailingElse.java b/test/metrics/ncss/ChainedIfElseWithTrailingElse.java new file mode 100644 index 00000000..4ffce9b0 --- /dev/null +++ b/test/metrics/ncss/ChainedIfElseWithTrailingElse.java @@ -0,0 +1,18 @@ +// Total NCSS = 12 + +class ChainedIfElse { // +1 + public int chainedIfElseMethod(int x) { // +1 + if(x > 10) { // +1 + return 100; // +1 + } else if(x > 5) { // +1 + return 5; // +1 + } else if (x > 0) { // +1 + return 1; // +1 + } else if (x == 0) { // +1 + return 0; // +1 + } + else { // +1 + return -1; // +1 + } + } +} \ No newline at end of file diff --git a/test/metrics/ncss/FinallyBlock.java b/test/metrics/ncss/FinallyBlock.java new file mode 100644 index 00000000..9e9724dc --- /dev/null +++ b/test/metrics/ncss/FinallyBlock.java @@ -0,0 +1,16 @@ +// Total NCSS = 6 + +class FinallyBlock { // +1 + private int x = 0; // +1 + + public void tryIncrement() { // +1 + try { + // Increment will never raise exception, + // but it can be replaced with something + // more dangerous. + x += 1; // +1 + } finally { // +1 + x = 0; // +1 + } + } +} \ No newline at end of file diff --git a/test/metrics/ncss/test_all_types.py b/test/metrics/ncss/test_all_types.py index ce5cdebe..2fd1509d 100644 --- a/test/metrics/ncss/test_all_types.py +++ b/test/metrics/ncss/test_all_types.py @@ -54,3 +54,21 @@ def testSimpleExample2(self): metric = NCSSMetric() res = metric.value(file) self.assertEqual(res, 18) + + def testChainedIfElse(self): + file = 'test/metrics/ncss/ChainedIfElse.java' + metric = NCSSMetric() + res = metric.value(file) + self.assertEqual(res, 11) + + def testChainedIfElseWithTrailingElse(self): + file = 'test/metrics/ncss/ChainedIfElseWithTrailingElse.java' + metric = NCSSMetric() + res = metric.value(file) + self.assertEqual(res, 12) + + def testFinallyBlock(self): + file = 'test/metrics/ncss/FinallyBlock.java' + metric = NCSSMetric() + res = metric.value(file) + self.assertEqual(res, 6)