Duplication when using CallerMemberName and serialization

Clean Code Report False-positive / False-negative...
,
1
  • What language is this for? C#
  • Which rule? Duplication
  • Are you using
    • SonarQube - 10.1 (build 73491)

Duplication problems emerge with the following code:

    [LoggerMessage(11, LogLevel.Information, "{auth} - {action}({param})")]
    public static partial void LogControllerAction(this ILogger logger, string auth, string? param = null, [CallerMemberName] string action = null!);

    public async Task<ActionResult> CreateResourceAsync([FromBody] MailboxModel? resource)
    {
        _logger.LogControllerAction(_identityService.GetUserName(), resource?.Uuid);
        _logger.DebugCreateResource(_identityService.GetUserName(), resource?.Uuid, JsonSerializer.Serialize(resource, _serializerOptions));

        await _mediator.Send(new NewResourceDispatchCommand(resource));

        return NoContent();
    }

    public async Task<ActionResult> CreateResourceAsync([FromBody] AppModel? resource)
    {
        _logger.LogControllerAction(_identityService.GetUserName(), resource?.Uuid);
        _logger.DebugCreateResource(_identityService.GetUserName(), resource?.Uuid, JsonSerializer.Serialize(resource, _serializerOptions));

        await _mediator.Send(new NewResourceDispatchCommand(resource));

        return NoContent();
    }

Why I think it shouldn’t:

  • the call to LogControllerAction can not be moved or the [CallerMemberName] will be lost
  • While MailboxModel and AppModel both inherit from a base model class, their serialization result won’t be the same hence it is not possible to refactor it.

To comply with the rule, I refactored the following way but honestly I fail to see the gain considering the actions done:

    public async Task<ActionResult> CreateResourceAsync([FromBody] AppModel? resource)
    {
        _logger.LogControllerAction(_identityService.GetUserName(), resource?.Uuid);
        await Create(resource, JsonSerializer.Serialize(resource, _serializerOptions));
        return NoContent();
    }

    private async Task Create(CoreResourceModel? model, string serialized)
    {
        _logger.DebugCreateResource(_identityService.GetUserName(), model?.Uuid, serialized);
        await _mediator.Send(new NewResourceDispatchCommand(model));
    }
3

Hi @Narfix

Thank you for your message.

Sonar is a tool, most times it really helps devs and development teams, and sometimes you need to use your own judgement on whether it’s better to tolerate some duplication, or that some code coverage is too expensive to be added etc.

In our analyzers, we also have some code duplication, mainly because Roslyn offers two separate namespaces for C# and VB .NET code analysis, and sometimes shared code for a .NET analyzer will only differ by the namespace at the top of a file (but class names are the same - e.g. Microsoft.CodeAnalysis.CSharp.Syntax.IdentifierNameSyntax vs Microsoft.CodeAnalysis.VisualBasic.Syntax.IdentifierNameSyntax). So we tolerate this.

However, we use generics to create base classes which contain the common logic, and we limit the C# and VB .NET child classes to only a few lines of code to provide the language specific types. This is something that we can do in our scenario.

I hope this answer helps you.

4

Hello @Andrei_Epure !
Thanks a lot for your reply

I mostly agree with you.
However if you use the SonarQube Quality Gate in your CI (Azure DevOps) :

  • Changing the CI configuration to ignore the quality gate / force merge would go against our ways
  • Changing the quality gate for this one PR in SonarQube would go against your “Clean as your code” way

:thinking:

Also, I may not have been very clear in my initial post. I have doubts on the Sonar analyzing correctly [CallerMemberName]. I’m not sure it understands it can not be factorized.

6

Currently, our copy-paste detection logic does not have such capabilities. I have forwarded your suggestion to our Product Management team.

Thank you.

1 Like