Цикл внутри try…catch, как написать красивый код?

#c_sharp #исключения #vkontakte_api #captcha

Код примерно следующий:

    public void AntigateAuthorize(int appId, string email, string password, Settings
settings, string antigateKey)
    {
        var authorizationCompleted = false;

        try
        {
            Authorize(appId, email, password, settings);
            authorizationCompleted = true;
        }
        catch (CaptchaNeededException exception)
        {
            var captchaSid = exception.Sid;
            var image = GetImageFromUrl(exception.Img.AbsoluteUri);

            try
            {
                const int numberOfAuthorizationRetries = 5;
                var retryAuthorizationsCount = numberOfAuthorizationRetries;
                do
                {
                    var anticaptcha = new AntiCaptcha(antigateKey);
                    try
                    {
                        var captchaKey = anticaptcha?.GetAnswer(image);

                        if (captchaKey != null)
                        {
                            Authorize(appId, email, password, settings, captchaSid,
captchaKey);
                            authorizationCompleted = true;
                        }
                        else
                        {
                            Logger.Error("Ответ от сервера Antigate не получен.");
                        }
                    }
                    catch (CaptchaNeededException)
                    {
                        Logger.Trace("Капча распознана неверно.");
                        anticaptcha.FalseCaptcha();
                        retryAuthorizationsCount--;
                        if (retryAuthorizationsCount == 0)
                        {
                            Logger.Trace("Заданное количество попыток авторизации
исчерпано.");
                            throw;
                        }
                    }
                } while (!authorizationCompleted && retryAuthorizationsCount > 0);
            }
            catch (AntigateErrorException aee)
            {
                Logger.Error("Ошибка Antigate: {0}", aee.Message);
            }
        }
        catch (VkApiAuthorizationException)
        {
            Logger.Error("Не удалось авторизовать приложение. Неправильный логин
или пароль.");
        }
        catch (VkApiException)
        {
            Logger.Error("В процессе попытки авторизации произошла неизвестная ошибка.");
        }
    }

    public static Image GetImageFromUrl(string url)
    {
        using (var stream = ((HttpWebResponse)((HttpWebRequest)WebRequest.Create(url)).GetResponse()).GetResponseStream())
            return stream == null ? null : Image.FromStream(stream);
    }


Проблема в том, если обрабатывать ошибку CaptchaNeededException однократно, то всё
получается, а если в цикле - получается плохо и некрасиво.
    

Ответы

Ответ 1

Можно попробовать разбить на мелкие функции. Например, так:

public void AntigateAuthorize(
        int appId, string email, string password,
        Settings settings, string antigateKey)
{
    try
    {
        try
        {
            Authorize(appId, email, password, settings);
        }
        catch (CaptchaNeededException exception)
        {
            AuthorizeWithCaptcha(appId, email, password, settings, exception);
        }
    }
    catch (AntigateErrorException)
    {
        // ?
    }
    catch (VkApiAuthorizationException)
    {
        Logger.Error(
            "Не удалось авторизовать приложение. Неправильный логин или пароль.");
    }
    catch (VkApiException)
    {
        Logger.Error(
            "В процессе попытки авторизации произошла неизвестная ошибка.");
    }
}


Вспомогательные функции:

private void AuthorizeWithCaptchaOnceImpl(
        int appId, string email, string password,
        Settings settings, CaptchaNeededException reason,
        AntiCaptcha anticaptcha)
{
    var captchaSid = reason.Sid;
    var image = GetImageFromUrl(reason.Img.AbsoluteUri);

    try
    {
        var captchaKey = anticaptcha.GetAnswer(image);

        if (captchaKey == null)
        {
            Logger.Error("Ответ от сервера Antigate не получен.");
            throw new NoAntigateResponceException();
        }

        Authorize(appId, email, password, settings, captchaSid, captchaKey);
    }
    catch (CaptchaNeededException)
    {
        Logger.Trace("Капча распознана неверно.");
        anticaptcha.FalseCaptcha();
        throw;
    }
}

void AuthorizeWithCaptcha(
        int appId, string email, string password,
        Settings settings, CaptchaNeededException exception)
{
    try
    {
        var anticaptcha = new AntiCaptcha(antigateKey);
        const int numberOfAuthorizationRetries = 5;

        CaptchaNeededException currentException = exception;

        for (int retry = 0; retry < numberOfAuthorizationRetries; retry++)
        {
            try 
            {           
                AuthorizeWithCaptchaOnceImpl(
                        appId, email, password,
                        settings, currentException, anticaptcha);
                return;
            }
            catch (CaptchaNeededException ex)
            {
                currentException = ex;
            }
            catch (NoAntigateResponceException ex)
            {
                // nothing to do, just retry
            }
        }
        throw new RetryNumberExceededException();
    }
    catch (AntigateErrorException aee)
    {
        Logger.Error("Ошибка Antigate: {0}", aee.Message);
        throw;
    }
}


Заметьте, что я немного поменял логику. Например, после неудачного распознавания
капчи теперь картинка меняется. Также, в результате превышения разрешённого количества
попыток распознавания выбрасывается исключение (хотя, возможно, вы захотите вернуть
bool из функции).


Ответ 2

Есть такая рекомендация: для упрощения читаемости кода блоки if/try/catch/finally/for/foreach/while
должны содержать как можно меньше кода, в идеале -- вызов одного метода.

Еще одна рекомендация: соблюдать минимальную вложенность кода.

Например, у вас есть код:

public void Foo()
{
    try
    {
        FooInternal();
    }
    catch (Exception1)
    {
        ...
    }
    catch (Exception2)
    {
        ...
    }
}


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



К вашему коду. Для внешнего блока try эти рекомендации у вас соблюдены:

try
{
    Authorize(appId, email, password, settings);
    authorizationCompleted = true;
}


Теперь осталось вынести содержимое catch (CaptchaNeededException exception) в отдельный
метод. В нем -- в свою очередь вынести содержимое блоков try/catch в отдельные методы.

От развесистой структуры блоков try/catch, к сожалению, совсем избавиться не удастся
-- такова логика.


Ответ 3

Я бы вынес логику отдельно, т.к.  в коде присутствуют другие методы не отвечающие
за авторизацию, нет следования принципу SRP и не будет reuse.

К примеру класс class Antigate Service, метод TryAuthenticate который будет возвращать
bool и out AuthenticationResult.

По крайней мере мне так видится, избавление в будущем от дублирования

И вообще следует избегать ветвления кода(даже если это try/catch)


Ответ 4

Переделал сейчас примерно следующим образом (убрал для примера лишние catch):

public void AuthorizeEx(
        int appId,
        string email,
        string password,
        Settings settings,
        int captchaRecognitionCount = MaxCaptchaRecognitionCount,
        AntiCaptcha antiCaptcha = null,
        CaptchaNeededException previousException = null)
    {
        if (captchaRecognitionCount < 0) return;

        var captchaSid = previousException?.Sid;
        var captchaKey = previousException == null ? null : antiCaptcha?.GetAnswer(GetImageFromUrl(previousException?.Img?.AbsoluteUri));

        try
        {
            Authorize(appId, email, password, settings, captchaSid, captchaKey);
        }
        catch (CaptchaNeededException currentException)
        {
            AuthorizeEx(appId, email, password, settings, captchaRecognitionCount
- 1, antiCaptcha, currentException);
        }
    }

    private static Image GetImageFromUrl([CanBeNull] string url)
    {
        if (url == null) return null;
        using (var stream = ((WebRequest.Create(url))?.GetResponse())?.GetResponseStream())
        {
            return stream == null ? null : Image.FromStream(stream);
        }
    }


Итог: код обработки капчи идёт отдельно, красиво. Из минусов - использование рекурсии,
но так ли это страшно, если я ограничиваю её уровень?
Если нехорошо, то как развернуть?