Sonar-dotnet: S138, S1541 and S3776 should take into consideration local functions

[andrei-epure-sonarsource]

Description

Local functions can be used to improve the readability of a method without exposing functionality to other class members. Thus, local functions should be included in a different metric regarding the complexity of functions. TBD

The following rules are to be discussed:
RSPEC-138 - Functions should not have too many lines of code
RSPEC-1541 - Functions should not be too complex
RSPEC-3776 - Cognitive Complexity of functions should not be too high

Original community thread

Related information

• SonarC# Version 7.13

[andrei-epure-sonarsource]

cc @nicolas-harraudeau-sonarsource

[andrei-epure-sonarsource]

Probably only static local functions should be excluded from a method's complexity calculations.

Because local functions that capture the context of the outer method can be difficult to follow (depending on how many variables are shared between the method and the local function)

[pavel-mikula-sonarsource]

Ideas for non-static local functions to decide if they should be treated separately or together with their containing method:

• read of method parameter => easy to understand
• write to method out/ref parameter => complex to understand
• rewrite of method parameter => complex to understand
• read of method local variable => easy to understand (equivalent to read parameter)
• write to method local variable => complex to understand (including collection manipulation)

[nicolas-harraudeau-sonarsource]

@andrei-epure-sonarsource +1 for only considering static local functions. However shouldn't we still consider that a method containing multiple complex static local functions is difficult to read and deserves a code smell issue?

[crypto-rsa]

@pavel-mikula-sonarsource good idea - I like to extract for example complex boolean conditions to local methods so that the body of the main method has a better readability. IMHO this shouldn't have a negative impact its complexity measure.

[egil]

Probably only static local functions should be excluded from a method's complexity calculations.

Because local functions that capture the context of the outer method can be difficult to follow (depending on how many variables are shared between the method and the local function)

I am not sure I agree with this, at least not in general. This is similar to having a class with private fields that multiple private methods share and use, with a single public method as the entry point.

I use local methods when I am in the realm between creating a new class and just having a large method. Then I can basically use local methods to break a few computation steps into a list of sub steps, potentially with a shared state.

[andrei-epure-sonarsource]

Thanks for the feedback @egil

This is similar to having a class with private fields that multiple private methods share and use, with a single public method as the entry point.

I agree. Did you read the suggestions from @pavel-mikula-sonarsource ?

---

@nicolas-harraudeau-sonarsource sorry, I just read your reply now.

However shouldn't we still consider that a method containing multiple complex static local functions is difficult to read and deserves a code smell issue?

I think that static local functions are equivalent to static private methods. They don't capture the context so I don't believe they add complexity to the method.

[egil]

I agree. Did you read the suggestions from @pavel-mikula-sonarsource ?

@andrei-epure-sonarsource nope, missed that, but now that I have, I agree with it :)

[paulhickman-a365]

I presume the discussion of how to treat complexity relating to static/non-static methods only applies to cognitive complexity, not cyclomatic?