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

Define and document the currently used MCC "McCabe" cyclomatic complexity (or change it) #41

Closed
3 tasks done
ResistantBear opened this issue Jan 23, 2024 · 14 comments · Fixed by #66
Closed
3 tasks done
Assignees
Labels
documentation Improvements or additions to documentation enhancement New feature or request priority:high Set by the PO only

Comments

@ResistantBear
Copy link
Collaborator

ResistantBear commented Jan 23, 2024

The currently implemented metric MCC does not equal the definition of the McCabe cyclomatic complexity and there is no further documentation provided for that metric.

Once clearly defined, check that there are sufficient test cases for this metric for each language.

Tasks:

  • Define and potentially rename MCC metric
  • Document the new definition
  • Add sufficient test cases
@ResistantBear ResistantBear changed the title Add definition of "McCabe Define and document the currently used "McCabe" cyclomatic complexity (or change it) Jan 23, 2024
@BridgeAR
Copy link
Member

The issue is that each programming language has some special handling and it is not as formalized due to that. Mostly, each implementation slightly deviates from each other. We can orient ourselves on the original suggestion as outlined in e.g., Wikipedia and see how it deviates to e.g., Sonar.

@ResistantBear ResistantBear changed the title Define and document the currently used "McCabe" cyclomatic complexity (or change it) Define and document the currently used MCC "McCabe" cyclomatic complexity (or change it) Jan 29, 2024
@ResistantBear
Copy link
Collaborator Author

  • what about Iterable.foreach(() => {}). How is this case handled e.g. in Sonar?

Originally posted by @ce-bo in #42 (comment)

According to https://docs.sonarsource.com/sonarqube/latest/user-guide/metric-definitions/, SonarQube does increase the complexity by one for "if, for, while, case, &&, ||, ?, ->". That means that in your example, it would actually increase the complexity by one, but not because of the "foreach"-call itself, but because of the lambda function with the arrow operator. Calling foreach with some non-lambda function should not do anything accordingly.

@ResistantBear
Copy link
Collaborator Author

Further discussion about MCC:
#42 (comment)

@ResistantBear
Copy link
Collaborator Author

ResistantBear commented Jan 29, 2024

Reading the definition used in https://docs.sonarsource.com/sonarqube/latest/user-guide/metric-definitions/ and having in mind the original defintion and the intention of @ce-bo to still include things like logical operators into the metric, which SonarQube does but the original definition of MCC, which is solely based on the control tree of a program, does not, I think that calculating the cyclomatic complexity per function in a similar way to SonarQube and using the maximum of the measured complexities inside one file as a "maximum of the cyclomatic complexities in the file" should be a viable option. As far as I understand, CodeCharta does also show such a value as "MCC" currently, with MCC meaning "maximum cyclomatric complexity" instead of McCable Complexity. By traversing the tree instead of using queries and resetting the count of "things that increase the complexity" when encountering a new function definition, that should be straight forward to implement.

On the other hand, we could also continue to just count these "things that increase the complexity" per file, which might give a better quantitative overview about the amount of logic inside the file, but less information about the complexity of the functions within it, as 100 functions with one if-statement each would have a really high value in this metric, but might not be that hard to understand. In this case I would recommend to no longer count function definitions as increasing the complexity, however. Having a long file with a few very long functions is probably more complex than having a large file separated into a lot of functions.

We could also include both options as separate metrics.

@ce-bo
Copy link
Collaborator

ce-bo commented Jan 31, 2024

The current approach for calculating the MCC metric is similar to the one of Sonar. It is based on the number of paths through the source code but also counts (as you already mentioned) operators like &&, ||, etc. and control structures like if, for, while, etc.
Also foreach is a control structure and should be counted as well.

I would like to rename the metric to "complexity" and add a definition to the Readme like I just did above.

@ResistantBear
Copy link
Collaborator Author

ResistantBear commented Jan 31, 2024

The current approach for calculating the MCC metric is similar to the one of Sonar. It is based on the number of paths through the source code but also counts (as you already mentioned) operators like &&, ||, etc. and control structures like if, for, while, etc. Also foreach is a control structure and should be counted as well.

I would like to rename the metric to "complexity" and add a definition to the Readme like I just did above.

Yes, but as I understand, Sonar counts the metric per function and not per file. The current implementation does add + 1 if it encounters a function definition, which should make the result match that of Sonar for files with a single function, but not for a file with more than one function.

The option of keeping the current implementation is discussed in the second paragraph of my comment above. The first suggestion would probably be closer to both Sonar and the original definition from McCabe, however.

@ce-bo
Copy link
Collaborator

ce-bo commented Feb 1, 2024

Sonar is counting the complexity per file also. Sonar adds +1 per function as we do. The results should be pretty much the same. I evaluated the results of counting the metric in metric-gardener in my master thesis.

Metric-gardner is calculating every metric on a file basis. This should not be changed. Calculating metrics on function level might be a feature request. Also, we should keep adding +1 per function definition, as a file with a lot of functions has more complexity than one with less functions. A file with too much functions might be a sign for splitting it up into smaller ones.

@ResistantBear
Copy link
Collaborator Author

ResistantBear commented Feb 1, 2024

Ah, I forgot about the MCC thing.
Interesting that Sonar does also do this calculation per file, it did sound as it is done per function in the documentation I linked above.

I tend to disagree that a function with many (necessarily shorter) functions is more complicated to understand than a file with only one very long function, assuming a file with the same complexity regarding the number of ifs, loops, etc.
Also the result of calculating the metric for each function and taking the maximum / sum / average / whatever of the MCCs of the functions inside the file would still result in one metric per file.

But as sonar does work similar to the current implementation, we should keep it that way and the other idea would be a suggestion for a separate kind of metric / new feature then to have a metric closer to the original McCabe cyclomatic complexity, possibly also per function then.

@ce-bo
Copy link
Collaborator

ce-bo commented Feb 1, 2024

Metric-gardener is file based and therefore this is a feature request that is not planned to be implemented soonish.

I move this back to backlog, so that we can finally start working on this :-)

@ResistantBear ResistantBear self-assigned this Feb 2, 2024
@ce-bo ce-bo added priority:high Set by the PO only documentation Improvements or additions to documentation enhancement New feature or request labels Feb 2, 2024
ResistantBear added a commit that referenced this issue Feb 2, 2024
ResistantBear added a commit that referenced this issue Feb 2, 2024
@ResistantBear
Copy link
Collaborator Author

We had the discussion whether or not to include "GOTO" in C#, etc., to the Complexity. It definitely makes the code harder to understand, but it is not included in the current definition nor in the McCabe Cyclomatic Complexity nor SonarQubes definition (at least for C# and C++, as it is not conditional there) as it does not branch the control flow / graph.

@ResistantBear
Copy link
Collaborator Author

ResistantBear commented Feb 2, 2024

A similar issue applies to the throw-statement. Currently, we count them with the Complexity-metric, while we do not count e.g. for return-statements.

@ce-bo
Copy link
Collaborator

ce-bo commented Feb 6, 2024

Let's reflect Sonars decision. I would not count it. It may be seen as a function call.

@ce-bo
Copy link
Collaborator

ce-bo commented Feb 6, 2024

I am not sure about Throw statements. I guess they can be counted. I remember that other metric tools are also counting it. But we can discuss this in person, to shorten this. Maybe it is not a good idea to do so.

Let's try to merge this PR as early as possible. And discuss this point afterwards.

@ResistantBear
Copy link
Collaborator Author

I am not sure about Throw statements. I guess they can be counted. I remember that other metric tools are also counting it. But we can discuss this in person, to shorten this. Maybe it is not a good idea to do so.

Let's try to merge this PR as early as possible. And discuss this point afterwards.

I have seen now that Sonar does count throw-statements for JS/TS and not for Java, C++, etc.
It also includes default-labels inside a switch-case for C but not for Java. Quite inconsistent.

For now, we do not include throw and goto statements then. We might change that later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request priority:high Set by the PO only
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants