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

Рейтинг: 3Ответов: 4Опубликовано: 21.04.2015

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

    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 однократно, то всё получается, а если в цикле - получается плохо и некрасиво.

c# исключения vkontakte-api captcha
Источник: Stack Overflow на русском

Ответы

▲ 4Принят

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

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 из функции).

▲ 1

Есть такая рекомендация: для упрощения читаемости кода блоки 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, к сожалению, совсем избавиться не удастся -- такова логика.

▲ 1

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

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

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

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

▲ 1

Переделал сейчас примерно следующим образом (убрал для примера лишние 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);
        }
    }

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