Metrics/AbcSize needs love #8037
Conversation
marcandre
force-pushed
the
abc
branch
3 times, most recently
from
fb4ab7a
to
bcb3836
Compare
|
For the record, the commit that "increased" the ABC complexity reduces it with these calculations (one less assignment, block call switched to a method call so same number of branches). Moreover, the origin of the 'offenses' being yielded instead of relying on the |
| let(:source) { <<~RUBY } | ||
| def method_name | ||
| x = foo # <1, 1, 0> | ||
| bar do # <1, 3, 0> (+1 for bar, +1 for non-empty block) |
There was a problem hiding this comment.
I'm not so sure that a block in itself should count as a branch. It says in abc_size_calculator.rb that a branch is "an explicit forward program branch out of scope". So a code block that can be called from outside the scope of the current method is not a branch IMO.
There was a problem hiding this comment.
Yeah, I'm not too sure. I read "For example, the B (branch) count is virtually identical to "cyclomatic complexity" here, so that's where I got my +1. But I'm not sure I can really make sense of that quote actually and I didn't understand that ABC is meant as a metric for size more than complexity. So yeah, maybe you're right and blocks shouldn't be counted.
I still need to make a PR for cyclomatic complexity that doesn't count blocks. Do you agree it's a +1 for all non-empty blocks as a block constitutes an additional disconnected node?
Also, yield should be counted as branches and I don't think they are.
There was a problem hiding this comment.
FWIW, maybe the quote meant to say "For example, the C (condition) count is virtually identical to "cyclomatic complexity". Maybe we should consider counting blocks as a "condition", in the sense that control flow is not linear at a block and entering it or not will be decided at runtime; exactly like and ifs or for. What do you think?
There was a problem hiding this comment.
I agree on yield. About cyclomatic complexity and blocks, I believe that's one of those things we can't know for sure with static analysis. The block could be another possible path through the method, but then again it might not be.
There was a problem hiding this comment.
When I said "additional disconnected node" I was wrong. Could be disconnected, if the block is captured and executed later, but typically it will behave like a loop/condition and be connected to the graph and add +1. In all cases it adds +1 to the cyclomatic complexity though.
There was a problem hiding this comment.
For Cyclomatic complexity, https://github.com/metricfu/Saikuro also counts +1 for blocks. I just can't find an example of Abc calculator though
marcandre
force-pushed
the
abc
branch
4 times, most recently
from
0fc2297
to
4eb0c0d
Compare
Mine too. Ideally we want to find a value that makes the cop behave like the current version, which has |
With the last few tweaks, autogen says that 18 is sufficient (see last temporary commit) |
marcandre
force-pushed
the
abc
branch
5 times, most recently
from
2db2793
to
ce31dd1
Compare
|
Bumping both by 1 creates only 9 more todos and removes one, doesn't seem so bad |
|
I'm moving the Cyclomatic issues to a different PR so this can be merged. Summary of PR as it stands: If default for |
marcandre
force-pushed
the
abc
branch
2 times, most recently
from
e5d1771
to
35fdf4e
Compare
|
I'd like to add |
marcandre
force-pushed
the
abc
branch
3 times, most recently
from
60b02f7
to
8e18bc8
Compare
|
I updated the PR to count +1 condition for methods like If default for AbcSize remains at 15, there are 30 new todos in the main gem... I hesitate between 16 and 17, so I tried 16.5 which gives +4 and -5, so that's what I'm proposing. |
|
I made a (hopefully 😅) final tweak to exclude some "non-assignments", either to |
config/default.yml
Outdated
| @@ -1884,7 +1884,7 @@ Metrics/AbcSize: | |||
| # The ABC size is a calculated magnitude, so this number can be an Integer or | |||
| # a Float. | |||
| IgnoredMethods: [] | |||
| Max: 15 | |||
| Max: 16.5 | |||
There was a problem hiding this comment.
I was thinking that since the max is pretty arbitrary anyway, you could choose a number that will cause very few new offenses for users, i.e. a hight number. But I guess you gave it a lot of thought.
There was a problem hiding this comment.
I just tried a few settings. 17 might be better and trigger less new failures (which could be frustrating) for people using our default.
There was a problem hiding this comment.
First of all, thank you for the research and discussion to difficult default value.
17 might be better and trigger less new failures (which could be frustrating) for people using our default.
IMHO, I often hear talk about disabling Metrics/AbcSize cop or increasing the maximum value. I think it is better to have Metrics/AbcSize cop work with increasing the default maximum value, rather than disabling the cop due to strict default.
So, if possible default value 17 seems to be a good value to me. (Of course, well thought out current value 16.5 is fine :-)
There was a problem hiding this comment.
Sure, bumped to 17
marcandre
force-pushed
the
abc
branch
2 times, most recently
from
63808c4
to
7e147b1
Compare
Now counts correctly ||=, &&=, multiple assignments, for, yield, iterating blocks. All &. count as a condition, unless it's on a repeated and unchanged lvar [see rubocop#8276]
It made no sense to me that this commit would actually increase the Abc complexity.
So I looked a bit into it, and the current Cop underestimates the complexity of a lot of things. This PR fixes many of them. It also tweaks
Metrics/CyclomaticComplexity.I personally dislike this metric.
obj.attributeandobj.my_method(arg, opt: 42, other_opt: 42); both are considered a branch.2 + 2is also a branch.nconditions andnbranches and 0 assignment is considered as complex as 0 conditions,nbranches andnassignments...But for those that actually like it, it should at least try to be relevant; failure in the calculation might lead someone to rewrite the method some strange to game the cop instead of actually addressing the issue.
Now the specs pass, but with the adjusted calculations there are 64 offenses where the code is now over the limit of 15. I think the max for these is 19.47
Possible solutions: