Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The ternary operator may be calculated incorrectly #19

Open
somethingSTRANGE opened this issue Nov 25, 2020 · 4 comments
Open

The ternary operator may be calculated incorrectly #19

somethingSTRANGE opened this issue Nov 25, 2020 · 4 comments

Comments

@somethingSTRANGE
Copy link

somethingSTRANGE commented Nov 25, 2020

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
Copy link
Owner

matkoch commented Nov 25, 2020

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
Copy link
Author

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

Twitter Question and Reply.

@czkraft
Copy link

czkraft commented Feb 23, 2022

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
Copy link
Owner

matkoch commented Feb 23, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants