search

matkoch / resharper-cognitivecomplexity

https://plugins.jetbrains.com/plugin/12024-cognitivecomplexity
MIT License
200 stars 7 forks source link

The ternary operator may be calculated incorrectly #19

Open somethingSTRANGE opened 3 years ago

somethingSTRANGE commented 3 years ago

I'm using Rider 2020.2.4 with CognitiveComplexity 2020.2.2.

According to Campbell's "Cognitive Complexity" paper linked on the plugin's GitHub page, shorthand (i.e., MyObj myObj = a?.myObj;) should be ignored, however in the section "Increment for breaks in linear flow", the ternary operator is specifically included with if and other conditionals, and it seems like it should generate a structural increment.

Increment for breaks in the linear flow

Another guiding principle in the formulation of Cognitive Complexity is that structures that break code’s normal linear flow from top to bottom, left to right require maintainers to work harder to understand that code. In acknowledgement of this extra effort, Cognitive Complexity assesses structural increments for:

  • Loop structures: for, while, do while, ...
  • Conditionals: ternary operators, if, #if, #ifdef, ...

It assesses hybrid increments for:

  • else if, elif, else, …

No nesting increment is assessed for these structures because the mental cost has already been paid when reading the if.

The ?. and ?? shorthand operators seem like they should be ignored, but not ?:.

My understanding is that the entire ?: should be treated as a single structural increment, so when nested inside a for loop, it should cost +2, which is less than an equivalent if-else costs at +3 (+2 for the structural if and +1 for the hybrid else). When not nested, the ?: would have a cost of +1, and the if-else would cost +2 (+1 structural and +1 hybrid).

In short, the ?: shorthand is still encouraged over the if-else, however it should have a complexity cost.

matkoch commented 3 years ago

Same here:

FYI, I let this review from the author and she approved all tests back then. If you have any questions/concerns I'd ask you to ping her on Twitter, and only report back here when she agrees. Her account is https://twitter.com/GAnnCampbell.

somethingSTRANGE commented 3 years ago

The author confirmed that ?: should be treated like an if.

Twitter Question and Reply.

czkraft commented 2 years ago

Hi @matkoch, so why don't you treat ?: like an if like G. Ann Campbell recommends? My problem now is that the complexity your plugin and SonarQube calculate are different. Can't you make a settings where the user can decide whether to count '?:' or not? (Nevertheless I really like your plugin!)

matkoch commented 2 years ago

I'm fine if anyone would send a pull request for this. Just keep in mind that there are many other short-hand syntaxes that should also be considered, like null coalescing, switch expressions, and compound assignments.