Please add option to ignore safe navigation in Metrics/CyclomaticComplexity

[fsateler]

Is your feature request related to a problem? Please describe.

With rubocop 0.86, CyclomaticComplexity now considers the safe navigation operator. Unfortunately, this makes some mapper code particularly prone to hitting the limit:

def to_hash_for_api obj
  {
    username: user&.name,
    email: user&.email,
    active_until: user&.active_until,
    role_name: user&.role&.name,
    role_group: user&.role&.group,
    permission_a: user&.role&.permission_a,
    permission_b: user&.role&.permission_b,
  }
end

This method has a cyclomatic complexity of 11, where previously it was 0.

Describe the solution you'd like

An option that when set, restores the pre-0.86 behavior of not considering the safe navigation.

Describe alternatives you've considered

Disabling the cop altogether is not optimal, because it sets limits for how var can methods grow.

[marcandre]

I'm sorry the multiple adjustments to this metric is causing you issues.

Could please you update your example to reflect better your usecase? In particular, your example has 5 times the same key definitely not what you intended, which makes it difficult to know if you actually intended the 5 repetitions of the same obj&.prop&....

[fsateler]

Could please you update your example to reflect better your usecase? In particular, your example has 5 times the same key definitely not what you intended

Sorry, done.

[marcandre]

Thanks, so much better 😄

I feel that if you used a local variable role = user&.role this would become be quite legible and you'd have your complexity down to 8.

One interesting point is the successive &. in a single expression, which are needed since I can't convince Matz yet and you can't write user&.role.name... Just curious, in your particular example, are there user for which user.role can be nil?

I'll quote @jonas054 :

broadly speaking, cyclomatic complexity is a metric that tells you how difficult is this to test.

The fact that you have these user&. mean that a test is needed for the case where user is nil. The sad thing here is that it is repeated 7 times, but clearly you don't need 7 tests for this. So maybe an option to count +1 for each unique receiver&. would make sense. In your example, this would give a complexity of +1 for user&. and +1 for role&..

I don't know what others think about this...

[fsateler]

I feel that if you used a local variable role = user&.role this would become be quite legible and you'd have your complexity down to 8.

True. But going from 11 to 8 is still going to make the cop fail 😉

Just curious, in your particular example, are there user for which user.role can be nil?

Yes, in our app that's how we distinguish people who have not been given any special permission (instead of an "empty" role).

The sad thing here is that it is repeated 7 times, but clearly you don't need 7 tests for this. So maybe an option to count +1 for each unique receiver&. would make sense. In your example, this would give a complexity of +1 for user&. and +1 for role&..

That would be nicer, indeed.

[marcandre]

ping @jonas054 / @bbatsov:
Should I discount repeated uses of &. or not (with a setting?)
This would impact AbcSize #8037 & PerceivedComplexity #8204 too, which are waiting for final word from @bbatsov

[jonas054]

Should I discount repeated uses of &. or not (with a setting?)

Yes. Based on your detailed explanation, @marcandre, that makes a lot of sense.

[marcandre]

Yes. Based on your detailed explanation, @marcandre, that makes a lot of sense.

Great.

Now I wonder: setting on or off by default?
Only discount for same variable&.method_call (like user&.methods above) which can't be factorized), or also for method_call&.other_method_call (which can, like user&.role&.permission above)? 😆

[marcandre]

I think that without a setting it is ok to count some_var&.foo ; some_var&.bar as a single condition (given that some_var is a variable and that it isn't reassigned in between). One could imagine Ruby doing that optimization internally. I'll start with that.

[marcandre]

I think that without a setting it is ok to count some_var&.foo ; some_var&.bar as a single condition (given that some_var is a variable and that it isn't reassigned in between). One could imagine Ruby doing that optimization internally. I'll start with that.

Implemented 🎉

I'm very satisfied with this solution. It requires no config, provides a justifiable discount for repeated my_var&.something and also provides a way for users to reduce their complexity by avoiding repeated method calls.

In the example above, it discounts 6 of the 7 uses of user&. and if further reduction is needed, role = user&.role could be used so that all role&. count as one condition. I feel this represents well the actual complexity of the method.

@fsateler : thank you for opening this issue and providing your real-life example! This will improve the CyclomaticComplexity metric, as well as PreceivedComplexity and AbcSize.