Split async method into two for code analysis?

Question

I have code:

public async Task DeleteColorSchemeAsync(ColorScheme colorScheme)
{
    if (colorScheme == null)
        throw new ArgumentNullException(nameof(colorScheme));

    if (colorScheme.IsDefault)
        throw new SettingIsDefaultException();

    _dbContext.ColorSchemes.Remove(colorScheme);
    await _dbContext.SaveChangesAsync();
}

One code analyzer recommends me to split this method to 2 methods:

Split this method into two, one handling parameters check and the other handling the asynchronous code

Am I correct, when I split this code in the following way?

public async Task DeleteColorSchemeAsync(ColorScheme colorScheme)
{
    if (colorScheme == null)
        throw new ArgumentNullException(nameof(colorScheme));

    if (colorScheme.IsDefault)
        throw new SettingIsDefaultException();

    await DeleteColorSchemeInternalAsync(colorScheme);
}

private async Task DeleteColorSchemeInternalAsync(ColorScheme colorScheme)
{
    _dbContext.ColorSchemes.Remove(colorScheme);
    await _dbContext.SaveChangesAsync();
}

What is different for the compiler? It sees two async methods, what is different from my first variant?

used code tool analyzator: sonarqube

Answer 1

Assuming you wanted to follow the code analysis advice, I would not make the first method async. Instead, it can just do the parameter validation, and then return the result of calling the second:

public Task DeleteColorSchemeAsync(ColorScheme colorScheme)
{
    if (colorScheme == null)
        throw new ArgumentNullException(nameof(colorScheme));

    if (colorScheme.IsDefault)
        throw new SettingIsDefaultException();

    return DeleteColorSchemeInternalAsync(colorScheme);
}

private async Task DeleteColorSchemeInternalAsync(ColorScheme colorScheme)
{
    _dbContext.ColorSchemes.Remove(colorScheme);
    await _dbContext.SaveChangesAsync();
}

All that said, in my opinion there's not really a strong justification to split the method like that. The SonarQube's rule, Parameter validation in "async"/"await" methods should be wrapped is IMHO overly cautious.

The compiler uses the same kind of transformation on async methods as it does for iterator methods. With an iterator method, there is value in doing parameter validation in a separate method, because otherwise it won't be done until the caller tries to get the first element in the sequence (i.e. when the compiler-generated MoveNext() method is called).

But for async methods, all of the code in the method up to the first await statement, including any parameter validation, will be executed on the initial call to the method.

The SonarQube rule appears to be based on a concern that until the Task is observed, any exception generated in the async method won't be observed. Which is true. But the typical calling sequence of an async method is to await the returned Task, which will observe the exception immediately on completion, which of course occurs when the exception is generated, and will happen synchronously (i.e. the thread won't be yielded).

I admit that this is not hard-and-fast. For example, one might initiate some number of async calls, and then use e.g. Task.WhenAll() to observe their completions. Without immediate parameter validation, you would wind up starting all of the tasks before realizing that one of the calls was invalid. And this does violate the general principle of "fail-fast" (which is what the SonarQube rule is about).

But, on the other hand, parameter validation failures are almost always due to user code incorrectness. I.e. they don't happen because of data input problems, but rather because the code was written incorrectly. "Fail-fast" is a bit of a luxury in that context; what's more important, to me anyway, is that the code be written in a natural, easy-to-follow way, and I'd argue that keeping everything in one method achieves that goal better.

So in this case, the advice being given by SonarQube isn't necessary to follow. You can just leave your async method as a single method, the way you had it originally without harming the code. Even more so than the iterator method scenario (which has similar arguments pro and con), there is IMHO just as much reason to leave the validation in the async method as to remove it to a wrapper method.

But if you do choose to follow SonarQube's advice, the example I provided above is IMHO a better approach than what you have (and indeed, is more in line with the detailed advice on SonarQube's documentation).

I will note that in fact, there's an even simpler way to express the code:

public Task DeleteColorSchemeAsync(ColorScheme colorScheme)
{
    if (colorScheme == null)
        throw new ArgumentNullException(nameof(colorScheme));

    if (colorScheme.IsDefault)
        throw new SettingIsDefaultException();

    _dbContext.ColorSchemes.Remove(colorScheme);
    return _dbContext.SaveChangesAsync();
}

I.e. don't make the implementation async at all. Your code doesn't need async because there's only one await and it occurs at the very end of the method. Since your code doesn't actually need control returned to it, there's not actually any need to make it async. Just do all the synchronous stuff you need to do (including parameter validation), and then return the Task you'd otherwise have awaited.

And, I'll note as well, this approach addresses both the code analysis warning, and keeps the implementation simple, as a single method with parameter validation built-in. Best of both worlds. :)