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

Add new check for nesting level #517

Merged
merged 1 commit into from
May 28, 2015

Conversation

Zetten
Copy link
Contributor

@Zetten Zetten commented May 22, 2015

Using a method inspired by Sonar-Java, check code for the depth of
nested blocks and raise issues accordingly.

@guwirth
Copy link
Collaborator

guwirth commented May 23, 2015

What is the difference to cyclomatic complexity (http://en.m.wikipedia.org/wiki/Cyclomatic_complexity)?

@jmecosta
Copy link
Member

They measure different things based on same concept? For example you can say you should only 5 levels of nesting in a function. And the complexity can still not flag any violation because its 10. Complexity gives the idea as if the function is only with one nested level.

@Zetten
Copy link
Contributor Author

Zetten commented May 23, 2015

Yeah, they are related, but cyclomatic complexity is more of a flat measure, e.g. lots of sequential unrelated if blocks = high complexity, while nesting level is a measure of a second dimension and is a good indicator for spaghetti code.

I think the nesting level is more indicative of a need for refactoring, but also easier refactoring. Pulling a deeply-nested block out to a separate function is easier and usually conceptually 'better' than splitting any general high-complexity method.

This PR does have one subtle point that may need consideration. In the AST, the way descendent nodes of ifStatement tokens are calculated mean that else-if blocks like:

if (cond1) {
  // do something
} else if (cond2) {
  // do something else
} else {
  // do a third thing
}

are actually equivalent to this:

if (cond1) {
  // do something
} else {
  if (cond2) {
    // do something else
  } else {
    // do a third thing
  }
}

So a series of else-ifs will increase the nesting level by one each time. But I think this is standard behaviour for most C++ static analysers (I recall one of my colleagues coded a rule for cppcheck which found the same thing).

However it would also be possible to catch the differing cases of an else if () {} and else { if () {} } and increase nesting level in the second case but not the first. It just takes an additional check on parent/preceding nodes in the AST.

Perhaps I should make this alternative behaviour a parameter for the check?

@guwirth
Copy link
Collaborator

guwirth commented May 23, 2015

If this metric is really measuring the nesting level (second case above) it makes sense. Would add no parameter because first case is equal to cyclomatic complexity. Especially with 'if/else if' and 'switch/case' the cyclomatic complexity metric is mostly wrong.

@guwirth guwirth added this to the M 0.9.4 milestone May 23, 2015
Using a method inspired by Sonar-Java, check code for the depth of
nested blocks and raise issues accordingly.
@Zetten
Copy link
Contributor Author

Zetten commented May 26, 2015

Okay, the if-else behaviour has been rectified.

The test case is reasonably comprehensive, but please do let me know if there are any glaring errors or omissions in there.

guwirth added a commit that referenced this pull request May 28, 2015
@guwirth guwirth merged commit 2b51517 into SonarOpenCommunity:master May 28, 2015
@guwirth guwirth mentioned this pull request Oct 25, 2015
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.

3 participants