将异步方法一分为二进行代码分析？

Question

我有代码：

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

一位代码分析器建议我将此方法拆分为 2 个方法：

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

当我按以下方式拆分此代码时，我是否正确？

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

编译器有什么不同？它看到两个异步方法，与我的第一个变体有什么不同？

使用过的代码工具分析器：sonarqube

Answer 1

假设您想遵循代码分析建议，我不会采用第一种方法async。相反，它可以只进行参数验证，然后 return 调用第二个的结果：

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

综上所述，在我看来，没有充分的理由像这样拆分方法。 SonarQube 的规则 Parameter validation in "async"/"await" methods should be wrapped 恕我直言过于谨慎。

编译器对 async 方法使用与迭代器方法相同的转换。使用迭代器方法，在单独的方法中进行参数验证是有价值的，因为否则直到调用者尝试获取序列中的第一个元素（即当 compiler-generated MoveNext() 方法被调用）。

但对于 async 方法，方法中直到第一个 await 语句的所有代码，包括任何参数验证，都将在对该方法的初始调用时执行。

SonarQube 规则似乎是基于这样一种担忧，即在观察到 Task 之前，不会观察到 async 方法中生成的任何异常。这是真的。但是一个async方法的典型调用顺序是await然后returned Task，它会在完成时立即观察到异常，这当然发生在异常是生成，并将同步发生（即不会产生线程）。

我承认这不是hard-and-fast。例如，一个人可能会发起一些 async 次调用，然后使用例如Task.WhenAll() 观察他们的完成情况。如果不立即进行参数验证，您将在意识到其中一个调用无效之前结束所有任务。这确实违反了 "fail-fast" 的一般原则（这就是 SonarQube 规则的内容）。

但是，另一方面，参数验证失败几乎总是由于用户代码不正确。 IE。它们不会因为数据输入问题而发生，而是因为代码编写不正确。 "Fail-fast" 在这种情况下有点奢侈；更重要的是，无论如何，对我来说，代码是以自然的方式编写的，easy-to-follow，我认为将所有内容都放在一个方法中可以更好地实现该目标。

所以在这种情况下，没有必要遵循 SonarQube 给出的建议。您可以将 async 方法保留为单个方法，就像您最初使用它的方式一样，而不会损害代码。甚至比迭代器方法场景（具有类似的赞成和反对论点）更重要的是，恕我直言，将验证留在 async 方法中的理由与将其删除到包装方法一样多。

但是如果您确实选择遵循 SonarQube 的建议，恕我直言，我上面提供的示例比您现有的方法更好（实际上，更符合 SonarQube 文档中的详细建议）。

我会注意到，其实还有一种更简单的代码表达方式：

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

即根本不要执行 async。您的代码不需要async，因为只有一个await，它出现在方法的最后。由于您的代码实际上不需要对其进行控制 return，因此实际上没有必要对其进行控制 async。只需执行您需要执行的所有同步操作（包括参数验证），然后 return 您原本等待的 Task。

而且，我还要指出，这种方法同时解决了代码分析警告，和保持实现简单，作为具有参数验证的单一方法built-in.两全其美。 :)

Answer 2

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

没有。拆分它们的正确方法是这样的：

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

（注意这里的入口方式是不是async）。

或者像这样，使用较新的局部函数：

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

存在此规则的原因是确保您在 usage exceptions 上尽快投掷。如果您不拆分逻辑并将验证留在 async 方法中，则只有在有人等待您返回的任务时才会抛出异常，这可能不会立即发生，具体取决于使用情况。

一个非常常见的流程是，当您想要同时触发多个任务并等待它们完成时，尽早抛出会有好处。由于在此流程中，您的 await 操作发生在任务被触发之后，您将得到一个异常，该异常可能与实际调用点相去甚远，从而使调试变得不必要地困难。

接受的答案还建议在只有一个异步操作要做的情况下直接返回任务，那就是结果值，但是这会在调试代码时引入重大问题，因为堆栈跟踪中省略了您的方法，使在整个流程中导航变得更加困难。这是更深入讨论此问题的视频： https://youtu.be/Q2zDatDVnO0?t=327

直接返回任务而不等待它只应该用于极其简单的“中继”类型的方法，其中父方法中的相关逻辑很少。

我建议您始终遵守该规则。

将异步方法一分为二进行代码分析？

Split async method into two for code analysis?

c#

async-await

sonarqube