Fix NPath computation#866
Conversation
WalkthroughThe NPath complexity calculation logic in Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant MvnFreeNPathMetric
participant ASTNode
TestSuite->>MvnFreeNPathMetric: compute_npath(ASTNode)
MvnFreeNPathMetric->>ASTNode: traverse node type
alt Expression Node
MvnFreeNPathMetric->>MvnFreeNPathMetric: _expression_npath(ASTNode)
alt Binary Operator
MvnFreeNPathMetric->>MvnFreeNPathMetric: _binary_expression_npath(ASTNode)
end
else If/Switch/Loop Node
MvnFreeNPathMetric->>MvnFreeNPathMetric: _if_npath/_switch_npath/_for_loop_npath/_while_loop_npath(ASTNode)
MvnFreeNPathMetric->>MvnFreeNPathMetric: _expression_npath(condition/control)
MvnFreeNPathMetric->>MvnFreeNPathMetric: _sequence_npath(children)
else Other Node
MvnFreeNPathMetric->>MvnFreeNPathMetric: _composite_npath(ASTNode)
end
MvnFreeNPathMetric-->>TestSuite: Return NPath value
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsaibolit/metrics/npath/main.py (3)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (9)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (2)
test/metrics/npath/javacode/EvenMoreComplicated.java (1)
14-20: Local variableais read before initialization – compilation will fail
boolean a, b = true;declaresawithout an initializer, yet the subsequentif (a || b)uses it.
Javac rejects reads of un-initialized local variables, which will break any build steps that attempt real compilation.-boolean a, b = true; +boolean a = false, b = true; // value doesn’t matter for metric, just must be initialisedtest/metrics/npath/javacode/OtherClass.java (1)
13-23: Undeclared identifiersi,j,MyClass– code will not compileThe body references variables and a class that aren’t declared anywhere in the file.
If the test harness ever switches to a compiler-backed parser this file will break.Fast fix:
- MyClass myObj = new MyClass(); - int result; - if(i % 2 == 0) { + int i = 0, j = 0; + int result; + if (i % 2 == 0) { result = i + j;…and either remove
MyClassor stub it.
♻️ Duplicate comments (3)
test/metrics/npath/javacode/ForWithAndCondition.java (1)
6-9: Same class-name collision as above.Rename
class Testor introduce a package to prevent clashes during bulk compilation.test/metrics/npath/javacode/IfWithIfElseInsideOuterElse.java (1)
6-6: Potential class-name duplication / default package issue.Same remark as for other files: unique class name or explicit package is preferable.
test/metrics/npath/javacode/NestedWhileLoops.java (1)
6-9: Class-name duplication in default package.
NestedWhileLoops.javaalso declares aTestclass. See earlier comments for mitigation.
🧹 Nitpick comments (20)
test/metrics/npath/javacode/EvenMoreComplicated.java (1)
30-30: Nit: clarify magic “288” comment or use an annotation blockA bare
// 288gives no context. Consider something self-describing (e.g.// NPATH: 288) so future readers immediately know what the number represents.test/metrics/npath/javacode/OtherClass.java (2)
3-8: Duplicate SPDX header – keep only one copyThe same copyright / licence banner appears twice.
Redundant headers add noise and hinder quick scans.
26-26: Same comment as above: make the trailing “3” self-describing
// NPATH: 3would be clearer.test/metrics/npath/javacode/EmptyWhileLoop.java (1)
11-11: Trailing magic number: prefer explicit tag, e.g.// NPATH: 2test/metrics/npath/javacode/Complicated.java (2)
7-8: Comment mixes cyclomatic and NPath metrics – may confuse readersLine 7 says “cyclomatic complexity 12”, while the new footer says
// 12(implied NPath).
Align the wording to avoid ambiguity, e.g.:- // This method has a cyclomatic complexity of 12 + // Expected NPath complexity: 12
30-30: Same note on magic numbers: make it explicit (// NPATH: 12).test/metrics/npath/javacode/SwitchSimpleWithoutDefault.java (1)
8-11: Add an explicitdefaultcase (even if empty) for completeness.Leaving out
defaultcan trigger style-check warnings and may weaken the example’s clarity compared with other switch test files that do include a default branch. A stub keeps behaviour unchanged while maintaining consistency:case 2: System.out.println("2"); break; + default: /* no-op */ + break;test/metrics/npath/javacode/IfWithOrCondition.java (2)
7-14: Add explicit access modifiers for test clarity
Package-private classes/methods are perfectly valid, but making the class and its methodpublicavoids compiler warnings when the file is compiled in isolation and keeps consistency with the rest of the test corpus.-class Test { - void validate(int x, int y) { +public class Test { + public void validate(int x, int y) { if (x == 0 || y == 0) { System.out.println("Zero detected"); } else { System.out.println("No zeros"); } } }
15-15: Keep metric annotations on a dedicated line comment
Some IDEs flag a trailing numeric literal (// 3) as a magic number. Prefixing with a small descriptor (e.g.// NPATH: 3) makes the intent explicit and silences such warnings without affecting metric logic.test/metrics/npath/javacode/TwoClassesDefinition.java (1)
6-8: File-name mismatch is fine but add a short comment
TwoClassesDefinition.javahosts two package-private classes. That’s legal, yet adding a quick note (// package-private by design) saves future confusion for newcomers skimming the code.test/metrics/npath/javacode/SwitchSimpleWithDefault.java (1)
15-15: Use an explicit tag for the metric value
Same remark as before – consider// NPATH: 3rather than a bare// 3.test/metrics/npath/javacode/EmptyInfiniteForLoop.java (2)
7-9: Remove the unused parameter to silence warnings
nis never referenced, which triggers an “unused parameter” warning in most static-analysis tools.- void empty(int n) { - for (;;); + void empty() { + for (;;); }This change does not alter the intended NPath (still 2).
11-11: Annotate metric value explicitly-// 2 +// NPATH: 2test/metrics/npath/javacode/ForWithSwitch.java (2)
9-14: Document intentional fall-through
Cases 1-3 deliberately fall through. Add// fall throughto avoid false-positive warnings from Checkstyle / SpotBugs.- case 1: System.out.println("1"); - case 2: System.out.println("2"); - case 3: System.out.println("3"); + case 1: System.out.println("1"); // fall through + case 2: System.out.println("2"); // fall through + case 3: System.out.println("3"); // fall through
18-18: Prefix metric literal-// 5 +// NPATH: 5test/metrics/npath/javacode/IfWithInnerIfElse.java (1)
6-6: Consider giving the class a unique name or package.Several test files declare a top-level class called
Testor similarly generic names in the default package. If the test harness ever compiles the whole folder, these collide.
Either add a lightweightpackage test.npath;declaration or ensure file-level unique class names.test/metrics/npath/javacode/WhileWithAndCondition.java (1)
6-9: DuplicateTestclass in default package.
WhileWithAndCondition.java,ForWithAndCondition.java,NestedWhileLoops.java, … all defineclass Testin the unnamed package. This compiles only when each file is compiled in isolation.
Adding a small package declaration or renaming avoids accidental clashes.-package (default) +package test.metrics.npath.snippets;test/metrics/npath/javacode/IfWithAndCondition.java (1)
15-15: Clarify the inline complexity markerThe lone comment
// 3is cryptic. Consider an explicit tag such as// NPATH: 3to aid automated harvesters.test/metrics/npath/javacode/ComplexIfElseWithNPathComplexityOf5.java (1)
25-25: Same remark as above: replace// 5with something descriptive like// NPATH: 5.aibolit/metrics/npath/main.py (1)
135-145: Consider adding comments to explain the switch complexity calculation.The new implementation handles edge cases well, but the logic for
null_statementsand the adjustments for missing cases/defaults could benefit from explanatory comments.def _switch_npath(self, node: ASTNode) -> int: 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 + # Account for fall-through cases (multiple case labels without break) null_statements = max(0, len(case_group.case) - 1) npath += self._node_npath(case_group.statements) + null_statements + # Add 1 if no cases exist, and 1 if no default exists return npath + int(not has_case) + int(not has_default)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
aibolit/metrics/npath/main.py(1 hunks)test/metrics/npath/javacode/ClassDefinition.java(1 hunks)test/metrics/npath/javacode/ComlpexForWithMultipleConstructs.java(1 hunks)test/metrics/npath/javacode/ComplexIfElseWithNPathComplexityOf5.java(1 hunks)test/metrics/npath/javacode/ComplexWithIfElseInsideIfElseBlocks.java(1 hunks)test/metrics/npath/javacode/Complicated.java(1 hunks)test/metrics/npath/javacode/EmptyInfiniteForLoop.java(1 hunks)test/metrics/npath/javacode/EmptyWhileLoop.java(1 hunks)test/metrics/npath/javacode/EvenMoreComplicated.java(2 hunks)test/metrics/npath/javacode/ForWithAndCondition.java(1 hunks)test/metrics/npath/javacode/ForWithBreakContinue.java(1 hunks)test/metrics/npath/javacode/ForWithIfInside.java(1 hunks)test/metrics/npath/javacode/ForWithOrCondition.java(1 hunks)test/metrics/npath/javacode/ForWithSwitch.java(1 hunks)test/metrics/npath/javacode/IfWithAndCondition.java(1 hunks)test/metrics/npath/javacode/IfWithIfElseInsideOuterElse.java(1 hunks)test/metrics/npath/javacode/IfWithInnerIfElse.java(1 hunks)test/metrics/npath/javacode/IfWithOrCondition.java(1 hunks)test/metrics/npath/javacode/NestedForLoops.java(1 hunks)test/metrics/npath/javacode/NestedSwitchStatements.java(1 hunks)test/metrics/npath/javacode/NestedWhileLoops.java(1 hunks)test/metrics/npath/javacode/OneIfElseStatement.java(1 hunks)test/metrics/npath/javacode/OneIfStatement.java(1 hunks)test/metrics/npath/javacode/OtherClass.java(2 hunks)test/metrics/npath/javacode/SimpleForLoop.java(1 hunks)test/metrics/npath/javacode/SimpleWhileLoops.java(1 hunks)test/metrics/npath/javacode/SwitchSimpleWithDefault.java(1 hunks)test/metrics/npath/javacode/SwitchSimpleWithoutDefault.java(1 hunks)test/metrics/npath/javacode/SwitchWithFallthrough.java(1 hunks)test/metrics/npath/javacode/TestSwitchEmpty.java(1 hunks)test/metrics/npath/javacode/TwoClassesDefinition.java(1 hunks)test/metrics/npath/javacode/WhileWithAndCondition.java(1 hunks)test/metrics/npath/javacode/WhileWithBreak.java(1 hunks)test/metrics/npath/javacode/WhileWithIf.java(1 hunks)test/metrics/npath/javacode/WhileWithOrCondition.java(1 hunks)test/metrics/npath/test_all_types.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
aibolit/metrics/npath/main.py (1)
Learnt from: SinbadTheSailor2005
PR: cqfn/aibolit#816
File: aibolit/patterns/loop_outsider/loop_outsider.py:0-0
Timestamp: 2025-06-08T11:50:22.673Z
Learning: In the aibolit codebase, ASTNodeType.ASSIGNMENT nodes have an attribute called `expressionl` (with 'l' suffix), not `expression`. This is confirmed in `aibolit/ast_framework/_auxiliary_data.py` and used consistently throughout existing patterns like mutable_index and many_primary_ctors.
🧬 Code Graph Analysis (11)
test/metrics/npath/javacode/OneIfStatement.java (1)
test/metrics/npath/javacode/OneIfElseStatement.java (1)
WithOneIf(6-14)
test/metrics/npath/javacode/ForWithAndCondition.java (4)
test/metrics/npath/javacode/ForWithOrCondition.java (1)
Test(6-12)test/metrics/npath/javacode/IfWithAndCondition.java (1)
Test(6-14)test/metrics/npath/javacode/IfWithOrCondition.java (1)
Test(6-14)test/metrics/npath/javacode/SimpleForLoop.java (1)
Test(6-12)
test/metrics/npath/javacode/SimpleWhileLoops.java (16)
test/metrics/npath/javacode/ComlpexForWithMultipleConstructs.java (1)
Test(6-19)test/metrics/npath/javacode/EmptyInfiniteForLoop.java (1)
Test(6-10)test/metrics/npath/javacode/EmptyWhileLoop.java (1)
Test(6-10)test/metrics/npath/javacode/ForWithAndCondition.java (1)
Test(6-12)test/metrics/npath/javacode/ForWithBreakContinue.java (1)
Test(6-18)test/metrics/npath/javacode/ForWithIfInside.java (1)
Test(6-14)test/metrics/npath/javacode/ForWithOrCondition.java (1)
Test(6-12)test/metrics/npath/javacode/IfWithAndCondition.java (1)
Test(6-14)test/metrics/npath/javacode/ForWithSwitch.java (1)
Test(6-17)test/metrics/npath/javacode/IfWithOrCondition.java (1)
Test(6-14)test/metrics/npath/javacode/NestedForLoops.java (1)
Test(6-14)test/metrics/npath/javacode/NestedWhileLoops.java (1)
Test(6-17)test/metrics/npath/javacode/SimpleForLoop.java (1)
Test(6-12)test/metrics/npath/javacode/SwitchSimpleWithDefault.java (1)
Test(6-14)test/metrics/npath/javacode/SwitchSimpleWithoutDefault.java (1)
Test(6-13)test/metrics/npath/javacode/SwitchWithFallthrough.java (1)
Test(6-16)
test/metrics/npath/javacode/OneIfElseStatement.java (1)
test/metrics/npath/javacode/OneIfStatement.java (1)
WithOneIf(6-12)
test/metrics/npath/javacode/ForWithIfInside.java (14)
test/metrics/npath/javacode/ComlpexForWithMultipleConstructs.java (1)
Test(6-19)test/metrics/npath/javacode/EmptyInfiniteForLoop.java (1)
Test(6-10)test/metrics/npath/javacode/ForWithAndCondition.java (1)
Test(6-12)test/metrics/npath/javacode/ForWithBreakContinue.java (1)
Test(6-18)test/metrics/npath/javacode/ForWithOrCondition.java (1)
Test(6-12)test/metrics/npath/javacode/IfWithAndCondition.java (1)
Test(6-14)test/metrics/npath/javacode/ForWithSwitch.java (1)
Test(6-17)test/metrics/npath/javacode/IfWithOrCondition.java (1)
Test(6-14)test/metrics/npath/javacode/NestedForLoops.java (1)
Test(6-14)test/metrics/npath/javacode/NestedWhileLoops.java (1)
Test(6-17)test/metrics/npath/javacode/SimpleForLoop.java (1)
Test(6-12)test/metrics/npath/javacode/SwitchSimpleWithDefault.java (1)
Test(6-14)test/metrics/npath/javacode/SwitchSimpleWithoutDefault.java (1)
Test(6-13)test/metrics/npath/javacode/SwitchWithFallthrough.java (1)
Test(6-16)
test/metrics/npath/javacode/WhileWithBreak.java (16)
test/metrics/npath/javacode/ComlpexForWithMultipleConstructs.java (1)
Test(6-19)test/metrics/npath/javacode/EmptyInfiniteForLoop.java (1)
Test(6-10)test/metrics/npath/javacode/EmptyWhileLoop.java (1)
Test(6-10)test/metrics/npath/javacode/ForWithAndCondition.java (1)
Test(6-12)test/metrics/npath/javacode/ForWithBreakContinue.java (1)
Test(6-18)test/metrics/npath/javacode/ForWithIfInside.java (1)
Test(6-14)test/metrics/npath/javacode/ForWithOrCondition.java (1)
Test(6-12)test/metrics/npath/javacode/IfWithAndCondition.java (1)
Test(6-14)test/metrics/npath/javacode/ForWithSwitch.java (1)
Test(6-17)test/metrics/npath/javacode/IfWithOrCondition.java (1)
Test(6-14)test/metrics/npath/javacode/NestedForLoops.java (1)
Test(6-14)test/metrics/npath/javacode/NestedWhileLoops.java (1)
Test(6-17)test/metrics/npath/javacode/SimpleForLoop.java (1)
Test(6-12)test/metrics/npath/javacode/SimpleWhileLoops.java (1)
Test(6-13)test/metrics/npath/javacode/SwitchSimpleWithDefault.java (1)
Test(6-14)test/metrics/npath/javacode/SwitchSimpleWithoutDefault.java (1)
Test(6-13)
test/metrics/npath/javacode/ComlpexForWithMultipleConstructs.java (1)
test/metrics/npath/javacode/ForWithSwitch.java (1)
Test(6-17)
test/metrics/npath/javacode/NestedForLoops.java (14)
test/metrics/npath/javacode/ComlpexForWithMultipleConstructs.java (1)
Test(6-19)test/metrics/npath/javacode/EmptyInfiniteForLoop.java (1)
Test(6-10)test/metrics/npath/javacode/ForWithAndCondition.java (1)
Test(6-12)test/metrics/npath/javacode/ForWithBreakContinue.java (1)
Test(6-18)test/metrics/npath/javacode/ForWithIfInside.java (1)
Test(6-14)test/metrics/npath/javacode/ForWithOrCondition.java (1)
Test(6-12)test/metrics/npath/javacode/IfWithAndCondition.java (1)
Test(6-14)test/metrics/npath/javacode/ForWithSwitch.java (1)
Test(6-17)test/metrics/npath/javacode/IfWithOrCondition.java (1)
Test(6-14)test/metrics/npath/javacode/NestedWhileLoops.java (1)
Test(6-17)test/metrics/npath/javacode/SimpleForLoop.java (1)
Test(6-12)test/metrics/npath/javacode/SwitchSimpleWithDefault.java (1)
Test(6-14)test/metrics/npath/javacode/SwitchSimpleWithoutDefault.java (1)
Test(6-13)test/metrics/npath/javacode/SwitchWithFallthrough.java (1)
Test(6-16)
test/metrics/npath/javacode/WhileWithAndCondition.java (3)
test/metrics/npath/javacode/ForWithAndCondition.java (1)
Test(6-12)test/metrics/npath/javacode/IfWithAndCondition.java (1)
Test(6-14)test/metrics/npath/javacode/SimpleWhileLoops.java (1)
Test(6-13)
test/metrics/npath/javacode/WhileWithOrCondition.java (3)
test/metrics/npath/javacode/ForWithOrCondition.java (1)
Test(6-12)test/metrics/npath/javacode/IfWithOrCondition.java (1)
Test(6-14)test/metrics/npath/javacode/SimpleWhileLoops.java (1)
Test(6-13)
test/metrics/npath/javacode/ForWithOrCondition.java (3)
test/metrics/npath/javacode/ForWithAndCondition.java (1)
Test(6-12)test/metrics/npath/javacode/ForWithIfInside.java (1)
Test(6-14)test/metrics/npath/javacode/SimpleForLoop.java (1)
Test(6-12)
🔇 Additional comments (24)
test/metrics/npath/javacode/ClassDefinition.java (1)
6-7: LGTM – minimal valid Java class, no issues foundtest/metrics/npath/javacode/TestSwitchEmpty.java (1)
1-12: LGTM! Well-structured test case for empty switch statement.The test case correctly implements an empty switch statement, which is a valid Java construct. The expected NPath complexity value of 2 is reasonable for this control flow structure.
test/metrics/npath/javacode/SimpleForLoop.java (1)
1-14: LGTM! Well-structured test case for simple for loop.The test case correctly implements a simple for loop with proper Java syntax. The expected NPath complexity value of 2 is reasonable for this control flow structure.
test/metrics/npath/javacode/SimpleWhileLoops.java (1)
1-15: LGTM! Well-structured test case for simple while loop.The test case correctly implements a simple while loop with proper Java syntax. The expected NPath complexity value of 2 is reasonable for this control flow structure.
test/metrics/npath/javacode/SwitchWithFallthrough.java (1)
8-14: Looks good – fall-through is clearly documented.Code is minimal, well-commented, and serves the intended test purpose without introducing ambiguity.
test/metrics/npath/javacode/NestedForLoops.java (1)
8-12: Nested-loop sample is fine.The snippet is concise and demonstrates the intended nesting scenario for NPath tests without unnecessary extras.
test/metrics/npath/javacode/ForWithBreakContinue.java (1)
8-16: LGTM – coversbreak/continuepaths cleanly.No functional or style issues spotted; the construct is clear for metric testing.
test/metrics/npath/javacode/ForWithIfInside.java (1)
8-12: Example is clear and self-contained.Implements the minimal pattern needed for the metric check with readable intent.
test/metrics/npath/javacode/SwitchSimpleWithDefault.java (1)
9-12: Nice concise switch block
The switch is minimal and correctly terminates each case. No issues spotted.test/metrics/npath/javacode/ForWithOrCondition.java (1)
1-14: LGTM! Well-structured test case for OR condition in loops.The implementation correctly demonstrates a for loop with an OR condition in the loop guard, which is appropriate for testing NPath complexity calculations for logical OR operators in loop conditions.
test/metrics/npath/javacode/ComplexWithIfElseInsideIfElseBlocks.java (1)
1-24: LGTM! Excellent test case for nested if-else complexity.The nested if-else structure correctly creates 4 distinct execution paths, making it an ideal test case for validating NPath complexity calculations for nested conditional constructs.
test/metrics/npath/javacode/ComlpexForWithMultipleConstructs.java (1)
6-21: LGTM! Good test case for multiple control flow constructs.The implementation correctly combines a for-each loop with conditional logic and a switch statement, providing appropriate coverage for testing NPath complexity calculations with multiple nested constructs.
test/metrics/npath/javacode/NestedSwitchStatements.java (1)
1-23: LGTM! Excellent test case for nested switch statements.The nested switch structure correctly demonstrates complex switch statement scenarios with proper break statements to prevent fall-through. The inline comment helpfully explains the path calculation, and the expected complexity of 5 is accurate.
test/metrics/npath/test_all_types.py (4)
162-177: LGTM! Well-designed helper methods for external file testing.The new helper methods
_filepathand_value_from_filepathprovide a clean abstraction for loading and testing external Java files, improving code maintainability and reducing duplication.
32-34: Inconsistency detected between AI summary and code.The AI summary mentions that the expected complexity value was updated from 200 to 288, but the code still shows 200. This needs clarification.
Please verify whether the expected complexity value for
EvenMoreComplicated.javashould be 200 or 288, and ensure the test assertion matches the actual expected value.Likely an incorrect or invalid review comment.
123-125: Test method references the typo filename.The test method references
ComlpexForWithMultipleConstructs.javawhich contains a typo. If the filename is corrected as suggested earlier, this reference should also be updated.Update the file reference once the Java file is renamed to fix the typo.
16-50: File path consistency confirmed; verify complexity assertions.All referenced
.javafiles undertest/metrics/npath/javacode/exist (the only missingooo1.javais intentionally used intestNonExistedFile). Please double-check the expected NPath complexity values in these tests:
- testLowScore (OtherClass.java):
res['data'][0]['complexity'] == 3- testMediumScore (Complicated.java):
res['data'][0]['complexity'] == 12- testHighScore (EvenMoreComplicated.java):
res['data'][0]['complexity'] == 200aibolit/metrics/npath/main.py (7)
112-112: LGTM! Good refactoring of the dispatcher.Adding
ASTNodeType.EXPRESSIONhandling and extracting the default handler to_composite_npathimproves code organization and readability.Also applies to: 115-115
118-120: LGTM! Clean extraction of default complexity calculation.The method provides a clear default behavior for nodes without specific handlers.
122-122: LGTM! Better use of built-in functions.Using
math.prodis more readable and efficient than manual multiplication.
148-148: Verify the additive complexity calculation for loops.Similar to the if-statement, the change from multiplication to addition deviates from traditional NPath complexity calculations.
Also applies to: 151-151
154-156: Consistent with other loop complexity calculations.The additive formula is consistently applied across all loop types.
158-161: LGTM! Clean expression handling implementation.The method provides a unified approach to handle different expression types with proper delegation to specialized handlers.
164-166: LGTM! Simplified binary expression complexity calculation.The implementation correctly adds extra complexity only for logical operators (&&, ||) which create additional execution paths.
|
@yegor256 Hello! Could you take a look when you have the time? |
|
@Error10556 Hello! Thank you for fixing the NPath complexity computation. Could you please move out |
|
@ivanovmg JFYI |
…th on FOR_node.control directly
|
@AntonProkopyev Thank you for the suggestions, I applied them. I will create a separate PR for the java test files later. |
|
@AntonProkopyev what's up with this one? good to go? |
There was a problem hiding this comment.
@Error10556 thank you very much for fixing it! I spent quite some time without success, but here comes the power of the open source and pdd!
It looks good to me. I have some optional comments though. Please have a look.
@yegor256 FYI
| else 1 | ||
| ) | ||
| return condition_npath * then_npath + else_npath | ||
| return condition_npath + then_npath + else_npath |
| 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) |
There was a problem hiding this comment.
Awesome, now switch statements seem to work right! However, from the readability standpoint, this statement is not that clear to me. As far as I understand, we must always add 1 for default case in npath metric, even if it is implicit. Nevertheless, here we seem to add 1 only when default is explicit. I wonder if you'd please clarify.
There was a problem hiding this comment.
Hello! So, in this statement, we ensure that we always add at least 1 for the default and at least 1 for the cases.
The idea is simple. First, we account for all the present cases and defaults. Then, we look back and retroactively "add" an empty case statement if there were none (+ int(not has_case)) and an empty default statement if there were none (+ int(not has_default)).
| ).strip() | ||
| assert self._value(content) == 2 | ||
| # NPath = 3 = 2 (for 2 CASEs) + 1 (for an empty DEFAULT) | ||
| assert self._value(content) == 3 |
There was a problem hiding this comment.
Thank you for fixing those, considering that you double checked with checkstyle!
There was a problem hiding this comment.
Yes, I have double-checked!
| # </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] |
There was a problem hiding this comment.
This is great that you made these tests pass! However, I recommend that we keep a comment that the actual npath complexity is 288, rather than 200 as stated in the java file.
|
@rultor merge |
|
@Error10556 many thanks! |
|
@Error10556 Thank you for your contribution! Here's the breakdown of your points: +16 base, +5.15 for 103 hits-of-code, -16 for no code review, -9.8 for 57 comments during review (exceeding the 8-comment threshold). Your total comes to 4 points, which aligns with our minimum reward policy. We appreciate your effort, but please ensure future contributions include a code review to maximize your points. Your running balance is now +4. Keep up the good work and remember to balance speed and quality in your future submissions! |
|
@ivanovmg Hey there, code review superstar! 🌟 You've just racked up a sweet +20 points for your awesome review. That's +12 for showing up and an extra +8 for those 47 thoughtful comments you dropped. Nice work on going above and beyond - you definitely dodged that 10-point deduction for low engagement! Your balance is now sitting pretty at +77. Keep up the great work, and who knows? You might just hit that 24-point max bonus next time! |
6 participants
I fixed the NPath complexity computation in
/test/metrics/npath/Complicated.java.In the process, I also discovered that 3 other tests (
test_switch_empty,test_switch_simple_without_default, andtest_switch_with_fallthrough) asserted an incorrect NPath value, so I edited them and clarified why the new asserted value was correct. The original Checkstyle agrees with me.Closes #862
Closes #863
Summary by CodeRabbit
Bug Fixes
Tests