Extend MvnFreeNPathMetric to handle while loops#861
Conversation
|
""" WalkthroughThe code introduces support for Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant MvnFreeNPathMetric
participant ASTNode
TestSuite->>MvnFreeNPathMetric: compute NPath (Java code with while loop)
MvnFreeNPathMetric->>ASTNode: traverse AST nodes
MvnFreeNPathMetric->>MvnFreeNPathMetric: _node_npath(node)
alt node is WHILE_STATEMENT
MvnFreeNPathMetric->>MvnFreeNPathMetric: _while_loop_npath(node)
MvnFreeNPathMetric->>MvnFreeNPathMetric: _condition_npath(node)
else other node types
MvnFreeNPathMetric->>MvnFreeNPathMetric: appropriate handler
end
MvnFreeNPathMetric-->>TestSuite: return NPath value
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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 (4)
✨ 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 (
|
| ASTNodeType.BINARY_OPERATION: self._binary_expression_npath, | ||
| } | ||
| handler = dispatcher.get(node.node_type, lambda n: self._sequence_npath(n.children)) | ||
| return handler(node) |
There was a problem hiding this comment.
Here I had to perform refactoring towards this dictionary dispatch pattern to avoid too many returns.
In particular, the linter was complaining about 7 return statements with a max of 6.
aibolit/metrics/npath/main.py:102:4: R0911: Too many return statements (7/6) (too-many-return-statements)
We can consider reverting and silencing the warning if the original implementation is considered more readable.
Alternatively, we can simply write resulting value to a variable result and then only return result once.
There was a problem hiding this comment.
@ivanovmg IMO the dictionary dispatching is pretty clean and easy to read. Maybe we could consider add explicit if when default handle is needed but the lambda is also OK for me.
There was a problem hiding this comment.
@AntonProkopyev I extracted a separate function default_handler. I hope it helps convey the message better than an anonymous function. Does it look OK?
|
@AntonProkopyev could you review, please? |
@yegor256 FYI |
@ivanovmg Will look till end of day. |
AntonProkopyev
left a comment
AntonProkopyev
left a comment
There was a problem hiding this comment.
@ivanovmg The PR is easy to read and understand. I have some minor considerations but the PR is well enough to get merged.
| return body_npath + self._condition_npath(node.control) | ||
|
|
||
| def _while_loop_npath(self, node: ASTNode) -> int: | ||
| body_npath = self._node_npath(node.body) | ||
| return body_npath + self._condition_npath(node) |
There was a problem hiding this comment.
@ivanovmg According to principle of least astonishment I suggest to calculate npath for condition before body part.
There was a problem hiding this comment.
@AntonProkopyev I moved the statements around to first calculate condition_npath and only then body_npath. Is it what you meant?
| ''').strip() | ||
| assert self._value(content) == 5 | ||
|
|
||
| def test_simple_while_loops(self) -> None: |
There was a problem hiding this comment.
@ivanovmg The tests are clear and sufficient! I would also suggest making sure that the tests from the previous implementation work for the new implementation.
There was a problem hiding this comment.
@AntonProkopyev the tests for the existing case files do not actually pass for MvnFreeNPathMetric as of now. I added the corresponding puzzles. Need some expertise in NPath complexity to fix the issues, I suppose. I tried to figure those out, but failed miserably.
| ASTNodeType.BINARY_OPERATION: self._binary_expression_npath, | ||
| } | ||
| handler = dispatcher.get(node.node_type, lambda n: self._sequence_npath(n.children)) | ||
| return handler(node) |
There was a problem hiding this comment.
@ivanovmg IMO the dictionary dispatching is pretty clean and easy to read. Maybe we could consider add explicit if when default handle is needed but the lambda is also OK for me.
@AntonProkopyev I have addressed the issues you raised. Please have a look when you can. |
This refactoring is used to solve the linting issue: ``` aibolit/metrics/npath/main.py:102:4: R0911: Too many return statements (7/6) (too-many-return-statements) ```
Slide condition_npath calculation upwards in response to the comment https://github.com/cqfn/aibolit/pull/861/files#r2198761207
|
@ivanovmg thanks! |
|
@ivanovmg Hey there! 👋 Great job on your contribution! You've earned +4 points for this one. Here's the breakdown: +16 as a base, +10.55 for your 211 hits-of-code (though we had to deduct 8 for going over 200), and -16 for no code review. We bumped it up to 4 to keep you motivated. Remember, code reviews are crucial, and keeping contributions smaller can boost your points next time. Your balance is now +115. Keep the code coming! |
4 participants
In this PR we extend
MvnFreeNPathMetricto handlewhileloops.All test cases are checked against
checkstyle.Closes #852
Summary by CodeRabbit
New Features
Tests