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

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

No. The correct way to split them would be like this:

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();
}

(note that the entry method is not async in this case).

Or like this, using the newer local functions:

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

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

    return DeleteColorSchemeAsync();

    async Task DeleteColorSchemeAsync()
    {
        _dbContext.ColorSchemes.Remove(colorScheme);
        await _dbContext.SaveChangesAsync();
    }
}

The reason this rule exists is to make sure you throw as soon as possible on usage exceptions. If you don't split the logic and leave validation inside the async method, the exception will only be thrown when someone awaits your returned task, which may not happen immediately depending on usage.

One very common flow where throwing early would be beneficial, is when you want to fire multiple tasks concurrently and await for their completion. Since on this flow your await action happens after the tasks are fired, you'll get an exception that is potentially very far from the actual point of the call, making it unnecessarily harder to debug.

The accepted answer also proposes returning the task directly in cases where you have only one async operation to do and that is the result value, however that introduces significant problems when debugging code, since your method is omitted from the stacktrace, making it harder to navigate in the entire flow. Here is a video that discusses this problem in more depth: https://youtu.be/Q2zDatDVnO0?t=327

Returning the task directly without awaiting it should only be done for extremely simple "relay"-type methods, where there is very little relevant logic in the parent method.

I'd advise following the rule at all times.