Sonar c# rule RSPEC-2327 makes incorrect assumption that processing should be halted

Rules and Languages Report False-positive / False-negative...
,
1

https://rules.sonarsource.com/csharp/tag/clumsy/RSPEC-2327
The compliant solution is not equevalent, and leaves no ability to perform step, handle failure, and continue performing steps, if the handler is the same.

for example:

try{
  cleanupDb(); //cleanup is not critical, there is a scheduled job to identify stale data and cleanup in batches
}catch(UserAccessDeniedException uadex){
  LogException(uadex);  //log the error which will create a ticket for operations
}
try{
 cleanupFileSystem();  //cleanup is not critical, there is a scheduled job to identify stale files and cleanup in batches
}catch(UserAccessDeniedException uadex){
  LogException(uadex); //log the error which will create a ticket for operations
}

in the above code, cleanupFileSystem is executed regardless of the outcome of cleanupDb, but the rule requires merging the try

try{
  cleanupDb();  //an exception thrown here prevents cleanupFileSystem from running
  cleanupFileSystem();
}
catch(UserAccessDeniedException uadex){
  LogException(uadex);
}
```
2 
try
{
    cleanupDb(); //cleanup is not critical, there is a scheduled job to identify stale data and cleanup in batches
}
catch (UserAccessDeniedException uadex)
{
    LogException(uadex); //log the error which will create a ticket for operations
}
try
{
    cleanupFileSystem(); //cleanup is not critical, there is a scheduled job to identify stale files and cleanup in batches
}
catch (UserAccessDeniedException uadex)
{
    LogException(uadex); //log the error which will create a ticket for operations
}

This code is unreadable, because you have two try-catch blocks in one method, and the rule advise you not to. I agree that the description of the rule can (and may be) should improve, and that the given advise will change the behavior, but the rule is right: this is a bad practice.

You could split up this method/introduce sub methods: TryCleanupDb() and TryCleanupFileSystem(), or make the original methods defensive by themselves.

3

“This code is unreadable, because you have two try-catch blocks in one method” that is fair and should be a rule in it’s own right, but is not what this specific rule is enforcing or recommending. The compliant example indicates it’s fine to have multiple try/catch as long as they’re handled differrently.

4

It might be fair to say that it should be a separate rule. However, if I read the reasoning given by Sonar, it exactly what they explained. I’m looking forward to their response. In the meantime, I would take the opportunity to refactor this piece of code, It can use some (and that’s why you have static code analysis in the first place).

5

Hello @Donald_Frederick

Sorry for the late answer.
Merging the two blocks changes the behaviour indeed, however with a bit of refactoring the readability and maintainability of the code could be increased, I would suggest the following way to solve your case:

    public void SomeMethod()
    {
        ExecuteCleanup(CleanUpDb);
        ExecuteCleanup(CleanupFileSystem);
    }

    public void ExecuteCleanup(Action cleanupAction)
    {
        try
        {
            cleanupAction();
        }
        catch (UserAccessDeniedException uadex)
        {
            LogException(uadex); //log the error which will create a ticket for operations
        }
    }

In the meanwhile I`ll start a discussion internally to see how we can improve the rule.

All the best,
Čaba

6

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.