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

implement explanation for cyclomatic complexity checks #1537

Merged

Conversation

ivangalkin
Copy link
Contributor

@ivangalkin ivangalkin commented Aug 8, 2018

  1. implement multi-location messages for squidsensor
  • move CxxReportIssue and CxxReportLocation from
    cxx-sensors to cxx-squid in order to satisfy dependencies
  1. introduce CxxCyclomaticComplexityCheck
  • this class is used for all scoped cyclomatic complexity checks

  • implement

    • ClassComplexityCheck (rule cxx:ClassComplexity)
    • FileComplexityCheck (rule cxx:FileComplexity)
    • FunctionComplexityCheck (rule cxx:FunctionComplexity)
  • they respect the scopes (important for nested classes)

  • they provide detailed information about complexity sources

This change addresses #1494:

  • cyclomatic complexity checks (done)
  • cognitive complexity checks (todo)

This change is Reviewable

@ivangalkin ivangalkin self-assigned this Aug 8, 2018
@guwirth guwirth added this to the 1.2 milestone Aug 9, 2018
@guwirth
Copy link
Collaborator

guwirth commented Aug 9, 2018

@ivangalkin also here my first question: are there relevant changes to to resulting metric values after the changes?

@ivangalkin
Copy link
Contributor Author

Hi @guwirth, metrics are not affected, only the checks

  • ClassComplexityCheck (same)
  • FileComplexityCheck (same)
  • FunctionComplexityCheck (fixed)

FunctionComplexityCheck produced previously a too large value. When called for SourceFunction, ChecksHelper.getRecursiveMeasureInt(sourceFunction, CxxMetric.COMPLEXITY) it always adds +1 because of function declaration; This makes the cyclomatic complexity of an empty function == 1; This is not correct, since we are interested in the body of the function only.

For the entire file (FileComplexityCheck) we do want to count the number of function declarations. So there it makes completely sense to add +1.

Regards

@guwirth
Copy link
Collaborator

guwirth commented Aug 9, 2018

@ivangalkin the reason why I'm always asking this: there are teams using the plugin to create hard/automated quality goals in there CI chain. Changing the values (especially increasing) could break their CI. We should at least inform in the release notes about such changes.

@ivangalkin
Copy link
Contributor Author

@guwirth yes, I'm aware of that. I didn't change the tests for metrics - they produces the same value. I've added a couple of new test cases for the checks, since I wanted to be sure, that the issue locations are correct. My test installation also shows the same value for complexity metrics.

@guwirth
Copy link
Collaborator

guwirth commented Aug 11, 2018

@ivangalkin

it always adds +1 because of function declaration; This makes the cyclomatic complexity of an empty function == 1; This is not correct, since we are interested in the body of the function only.

Think SQ Java is also doing this. Maybe you can compare it with Java code results. Should be consistent.

https://docs.sonarqube.org/plugins/servlet/mobile?contentId=11638440#content/view/11638440

depends on #1535

Merged

@ivangalkin
Copy link
Contributor Author

Hi @guwirth,

you are right about the Java behavior. I've analyzed the official example and made the following conclusions:

  1. The official example is wrong: it's cyclomatic complexity is not 5 but 4
  2. Nevertheless the function declaration itself is added to the cyclomatic complexity.
  3. For the cognitive complexity this is not true: function declaration is excluded from the cognitive complexity score. So in case of official example it's 3.

For 2 and 3 please see the screenshots.

cyclomatic_complexity

cognitive_complexity

By the way, the difference in cyclomatic and cognitive complexity w.r.t. the function declaration is documented in the official doc https://www.sonarsource.com/docs/CognitiveComplexity.pdf

I'll correct my current pull request. The next 3 weeks I'm on vocation and will work on #1494 only sporadically.

Regards,

@guwirth
Copy link
Collaborator

guwirth commented Aug 14, 2018

The next 3 weeks I'm on vocation

@ivangalkin wish you a relaxing vacation!

1. implement multi-location messages for squidsensor
* move CxxReportIssue and CxxReportLocation from
  cxx-sensors to cxx-squid in order to satisfy dependencies

2. introduce CxxCyclomaticComplexityCheck
* this class is used for all scoped cyclomatic complexity checks
* implement
  * ClassComplexityCheck
  * FileComplexityCheck
  * FunctionComplexityCheck

* they respect the scopes (important for nested classes)
* they provide detailed information about complexity sources
* FileComplexityCheck previously had a wrong calculation:
  cyclomatic complexity of functions was always <correct score> + 1
  -> fixed now

This change addresses SonarOpenCommunity#1494:

* cyclomatic complexity checks (done)
* cognitive complexity checks (todo, depends on SonarOpenCommunity#1535
* apply comment about the cyclomatic complexity of functions:
  the function defintion is always included in the final complexity score

* see https://docs.sonarqube.org/plugins/servlet/mobile?contentId=11638440#content/view/11638440
@ivangalkin
Copy link
Contributor Author

Hi @guwirth!

wish you a relaxing vacation!

Thank you very much! I've applied you comment about the correct calculation of cyclomatic complexity w.r.t. function definition. Now it's done according to the official example and in the same way, Java plugin calculates it.

Regards

Copy link
Collaborator

@guwirth guwirth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivangalkin
Copy link
Contributor Author

Hi All,

are there some objections to merge this change? I've already implemented the cognitive complexity check (planned as follow-up PR) and would like to finalize #1494.

Thx

@guwirth guwirth merged commit bd5adde into SonarOpenCommunity:master Aug 26, 2018
@guwirth
Copy link
Collaborator

guwirth commented Aug 26, 2018

@ivangalkin thx for providing this. Maybe you can have a look to technical debt on https://sonarcloud.io/dashboard?id=org.sonarsource.sonarqube-plugins.cxx%3Acxx and fix it in one of the next PR. Thx.

@ivangalkin
Copy link
Contributor Author

@guwirth thank you for the hint; I'll provide the "fix quality flows" change soon

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

Successfully merging this pull request may close these issues.

2 participants