S138, S1541 and S3776 should take into consideration non-static 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?

[Sergiu-Cosmin]

2 years later, is this topic still being discussed ? we cannot use static local functions because sonarqube is complaining about cyclomatic complexity, this should really not be the case in my opinion

can we get a timeframe when will this be fixed please ?

[cjbush]

Hi from the future. This flaw in these quality gates is particularly bad when dealing with .NET 6's new minimal APIs, where you might have route handlers, DI code, and configuration building all done in a single Program.cs. Consider:

var builder = WebApplication.CreateBuilder(args);
ConfigureServices(builder.Services, builder.Configuration);

var app = builder.Build();
AddRoutes(app);
app.Run();

static void ConfigureServices(IServiceCollection services, IConfiguration configuration) 
{
     //100 lines of binding dependencies for your web app
}

static void AddRoutes(WebApplication app) 
{
     if (app.Environment.IsDevelopment())
    {
        app.UseSwagger();
        app.UseSwaggerUI();
    }
    app.UseHttpsRedirection();
    //a bunch more logic for setting up health checks, middleware, etc
    app.MapGet("/api/status", async() {
         //30 lines of actual route handling code
    });
    app.MapGet("/api/status2", HandleStatus2);
}

static IResult HandleStatus2() {
    //30 more lines of actual route handling code with conditionals and everything
}

Sonar flagging this as having high cognitive complexity due to it lumping the local static methods in with the main method more or less renders minimal APIs useless for anything more than a trivial use case. Please fix this.

[andrei-epure-sonarsource]

sorry for the late answer @paulhickman-a365. I believe the discussion makes sense for both.

@Sergiu-Cosmin @cjbush sorry, this went of our radar. In any case, the discussion here involves too many pieces (especially non-static local functions vs static local functions), so I opened #5625 to exclude static local functions from computing cognitive complexity. This should reduce most of the noise and it's easier to implement than the logic for non-static local functions (which can make the code quite hard to read when abused).

@cjbush regarding top-level statements, I've opened #5624 where I encourage you to give some feedback.

Thanks a lot for contributing to this thread.

[andrei-epure-sonarsource]

An update: static local functions are now excluded from computing cognitive complexity - see release notes https://github.com/SonarSource/sonar-dotnet/releases/tag/8.40.0.48530 .