Разделить асинхронный метод на два для анализа кода? - PullRequest
Новая
       12

Разделить асинхронный метод на два для анализа кода?

0 голосов
/ 06 июля 2019

У меня есть код: 

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 метода:

Разделить этот метод на два, одна проверка параметров обработки идругая обработка асинхронного кода

Прав ли я, когда я разделяю этот код следующим образом? 

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

1 Ответ

2 голосов
/ 06 июля 2019

Предполагая, что вы хотите следовать советам по анализу кода, я бы не стал использовать первый метод async. Вместо этого он может просто выполнить проверку параметров, а затем вернуть результат вызова второго: 

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 Проверка параметров в методах "async" / "await" должна быть перенесена ИМХО слишком осторожна.

Компилятор использует тот же тип преобразования для async методов, что и для методов итератора. При использовании метода итератора есть значение при проверке параметров в отдельном методе, поскольку в противном случае это не будет сделано до тех пор, пока вызывающая программа не попытается получить первый элемент в последовательности (т. Е. Когда вызывается сгенерированный компилятором метод MoveNext() ).

Но для методов async весь код в методе до первого оператора await, включая проверку любого параметра, будет выполняться при первоначальном вызове метода.

Правило SonarQube, похоже, основано на опасении, что до тех пор, пока Task не будет соблюдено, любое исключение, сгенерированное в методе async, не будет соблюдаться. Что является правдой. Но типичной вызывающей последовательностью метода async является await возвращенного Task, который будет наблюдать исключение сразу после завершения, что, конечно, происходит, когда генерируется исключение, и происходит синхронно (т. Е. Поток выиграл не уступит).

Я признаю, что это не сложно и быстро. Например, можно инициировать некоторое количество вызовов async, а затем использовать, например, Task.WhenAll() соблюдать их доработки. Без немедленной проверки параметров вы бы запустили все задачи, прежде чем осознали, что один из вызовов был недействительным. И это действительно нарушает общий принцип «fast-fast» (о чем говорит правило SonarQube).

Но, с другой стороны, сбои проверки параметров почти всегда происходят из-за неправильного пользовательского кода. То есть они происходят не из-за проблем с вводом данных, а из-за того, что код был написан неправильно. «Безотказный» - это немного роскошь в этом контексте; в любом случае, для меня более важно, чтобы код был написан естественным и простым для понимания способом, и я бы сказал, что хранение всего в одном методе позволяет достичь этой цели лучше.

Так что в этом случае совету, который дает 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, и это происходит в самом конце метода. Поскольку вашему коду на самом деле не требуется возвращать элемент управления, на самом деле нет необходимости делать его async. Просто выполните все необходимые синхронные операции (включая проверку параметров), а затем верните Task, которого вы в противном случае ожидали.

И я также отмечу, что этот подход учитывает как предупреждение анализа кода, и делают реализацию простой, как единый метод со встроенной проверкой параметров. Лучшее из обоих миров. :)

...