«Насколько плох» несвязанный код в блоке try-catch-finally?

11

Это связано с Q: Является ли использование оператора finally для выполнения работы после возврата плохим стилем / опасным?

В ссылочной версии Q код finally связан с используемой структурой и необходимостью предварительной выборки. Мой вопрос немного другой, и я считаю, что он уместен для широкой аудитории. Мой конкретный пример - приложение на C # winform, но это также относится и к окончательному использованию C ++ / Java.

Я заметил довольно много блоков try-catch-finally, где много кода, не связанного с исключениями и обработкой / очисткой исключений, скрытых внутри блока. И я признаю свою предвзятость к наличию очень узких блоков try-catch-finally с кодом, тесно связанным с исключением и обработкой. Вот несколько примеров того, что я вижу.

Блоки try будут иметь множество предварительных вызовов и устанавливаемых переменных, ведущих к коду, который может выдать. Информация журнала будет настроена и запущена в блоке try.

Наконец, блоки будут иметь вызовы форматирования формы / модуля / элемента управления (несмотря на то, что приложение собирается завершиться, как указано в блоке catch), а также создание новых объектов, таких как панели.

Грубо говоря:

    methodName (...)
    {
        пытаться
        {
            // Много кода для метода ...
            // код, который может бросить ...
            // Много кода для метода и возврата ...
        }
        поймать (что-то)
        {// обработать исключение}
        наконец
        {
            // некоторая очистка из-за исключения, закрытие вещей
            // больше кода для материала, который был создан (игнорируя, что любые исключения могли выдать) ...
            // возможно создать еще несколько объектов
        }
    }

Код работает, поэтому в этом есть какая-то ценность. Это не очень хорошо инкапсулировано, и логика немного запутанная. Я (до боли) знаком с рисками, связанными с переносом кода, а также с рефакторингом, поэтому мой вопрос сводится к желанию узнать опыт других людей с аналогично структурированным кодом.

Плохой стиль оправдывает внесение изменений? Кто-нибудь сильно сгорел от подобной ситуации? Не могли бы вы поделиться подробностями этого неудачного опыта? Оставь это, потому что я чрезмерно реагирую, и это не так уж плохо в стиле? Получите пользу от обслуживания уборки?

Сообщество
источник
Тэг "C ++" здесь не принадлежит, так как C ++ не имеет (и не нуждается) finally. Все хорошие применения подпадают под RAII / RRID / SBRM (какой бы аббревиатурой вы не хотели).
Дэвид Торнли
2
@DavidThornley: Помимо примера использования ключевого слова finally, остальная часть вопроса прекрасно подходит для C ++. Я разработчик C ++, и это ключевое слово меня не смутило. И, учитывая, что у меня аналогичные разговоры с моей собственной командой на полурегулярной основе, этот вопрос очень важен для того, что мы делаем с C ++
DXM
@DXM: у меня проблемы с визуализацией (и я знаю, что finallyделает в C #). Что такое эквивалент C ++? Я думаю о коде после catch, и то же самое относится и к коду после C # finally.
Дэвид Торнли
@DavidThornley: код finally - это код, который выполняется независимо от того, что .
orlp
1
@ nightcracker, это не совсем правильно. Это не будет выполнено, если вы делаете Environment.FailFast(); оно может быть не выполнено, если у вас есть неперехваченное исключение. И это становится еще сложнее, если у вас есть блок итератора, finallyкоторый вы выполняете вручную.
свик

Ответы:

10

У меня была очень похожая ситуация, когда мне приходилось иметь дело с ужасным устаревшим кодом Windows Forms, написанным разработчиками, которые явно не знали, что они делают.

Прежде всего, вы не переигрываете. Это плохой код. Как вы сказали, блок catch должен быть об отмене и подготовке к остановке. Не время создавать объекты (особенно Панели). Я даже не могу объяснить, почему это плохо.

Что, как говорится...

Мой первый совет: если он не сломан, не трогай его!

Если ваша задача состоит в том, чтобы поддерживать код, вы должны сделать все возможное, чтобы не нарушать его. Я знаю, это больно (я был там), но вы должны сделать все возможное, чтобы не сломать то, что уже работает.

Мой второй совет: если вам нужно добавить больше возможностей, постарайтесь максимально сохранить существующую структуру кода, чтобы не нарушать код.

Пример: если есть отвратительный оператор switch-case, который, по вашему мнению, может быть заменен правильным наследованием, вы должны быть осторожны и дважды подумать, прежде чем решиться начать что-то менять.

Вы определенно найдете ситуации, когда рефакторинг является правильным подходом, но будьте осторожны: рефакторинг кода с большей вероятностью может привести к ошибкам . Вы должны принять это решение с точки зрения владельцев приложений, а не с точки зрения разработчика. Таким образом, вы должны подумать, стоит ли усилий (денег), необходимых для решения проблемы, рефакторинга или нет. Я много раз видел, как разработчик тратил несколько дней на исправление чего-то, что на самом деле не сломалось только потому, что он думает, что «код уродлив».

Мой третий совет: вы обожжетесь, если нарушите код, не имеет значения, виноват он или нет.

Если вас наняли для обслуживания, на самом деле не имеет значения, разваливается ли приложение, потому что кто-то другой принял плохие решения. С точки зрения пользователя это работало раньше, и теперь вы сломали его. Вы сломали это!

Джоэл очень хорошо описывает свою статью, объясняя несколько причин, по которым вам не следует переписывать устаревший код.

http://www.joelonsoftware.com/articles/fog0000000069.html

Так что вы должны очень плохо относиться к такому коду (и никогда не должны писать что-то подобное), но поддержание его - это совсем другой монстр.

О моем опыте: я должен был поддерживать код в течение приблизительно 1 года, и в конце концов я смог переписать его с нуля, но не все сразу.

Случилось так, что код оказался настолько плохим, что новые функции было невозможно реализовать. Существующее приложение имело серьезные проблемы с производительностью и удобством использования. В конце концов меня попросили внести изменения, которые заняли бы у меня 3-4 месяца (в основном потому, что работа с этим кодом заняла у меня намного больше времени, чем обычно). Я думал, что смогу переписать весь этот кусок (включая реализацию желаемой новой функции) примерно через 5-6 месяцев. Я принес это предложение заинтересованным сторонам, и они согласны переписать его (к счастью для меня).

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

Мой подход состоял в том, чтобы переписать его по частям. Сначала я заменил весь пользовательский интерфейс (Windows Forms), затем я начал заменять коммуникационный уровень (вызовы веб-службы), а затем я заменил всю реализацию сервера (это было приложение типа «толстый клиент / сервер»).

Спустя пару лет это приложение превратилось в прекрасный ключевой инструмент, используемый всей компанией. Я на 100% уверен, что это было бы невозможно, если бы я не переписал все это.

Несмотря на то, что я смог сделать это, важная часть заключается в том, что заинтересованные стороны одобрили его, и я смог убедить их, что это стоит денег. Поэтому, пока вы должны поддерживать существующее приложение, сделайте все возможное, чтобы ничего не сломать, и если вы сможете убедить владельцев в преимуществах его переписывания, попробуйте сделать это как Jack the Ripper: по частям.

Alex
источник
1
+1. Но последнее предложение относится к серийному убийце. Это действительно необходимо?
MarkJ
Мне нравится часть этого, но в то же время оставляя сломанные дизайны на месте, а добавление вещей часто приводит к худшему дизайну. Лучше разобраться, как это исправить и исправить, хотя, конечно, взвешенно.
Рики Кларксон
@RickyClarkson Я согласен. Трудно понять, когда вы перестарались (рефакторинг на самом деле не нужен) и когда вы действительно наносите вред приложению, добавляя изменения.
Алекс
@RickyClarkson Я так согласен с этим, что даже смог переписать проект, в котором участвовал. Но долгое время мне приходилось аккуратно добавлять вещи, стараясь нанести минимально возможный ущерб. Если запрос не состоит в том, чтобы исправить то, что основной причиной является плохое проектное решение (что обычно не так), я бы сказал, что дизайн приложения не должен затрагиваться.
Алекс
@RickyClarkson Это было частью опыта сообщества, к которому я хотел подключиться. Объявление всего устаревшего кода как плохого является в равной степени плохим антишаблоном. Однако в этом коде, над которым я работаю, есть вещи, которые на самом деле не должны быть частью блока finally. Отзывы Алекса и ответ - это то, о чем я хотел прочитать.
2

Если нет необходимости менять код, оставьте его как есть. Но если вам нужно изменить код, потому что вы должны исправить какую-то ошибку или изменить некоторые функции, вам придется изменить ее, нравится вам это или нет. ИМХО, это часто более безопасно и менее подвержено ошибкам, если вы сначала реорганизуете его в более мелкие, более понятные части, а затем добавляете функциональность. Когда у вас есть инструменты автоматического рефакторинга, это можно сделать относительно безопасно, с очень небольшим риском появления новых ошибок. И я рекомендую вам получить копию книги Майкла Фезерса

http://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052

что даст вам ценные советы, как сделать код более тестируемым, добавить модульные тесты и избежать взлома кода при добавлении новых функций.

Если вы все сделаете правильно, надеюсь, ваш метод станет достаточно простым, так что try-catch-finally больше не будет содержать никакого несвязанного кода в неправильных местах.

Док Браун
источник
2

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

try {
     method1();  // throws
     method2();
     method3();
     method4();
     method5();
 } catch(e) {
     // handle error
 }
 method6();

против

 try {
      method1();  // throws
 }
 catch(e) {
      // handle error
 }
 if(! error ) {
     method2();
     method3();
     method4();
     method5();
 }
 method6();

Во втором коде вы обрабатываете исключения как коды возврата. Концептуально это идентично:

rc = method1();
if( rc != error ) {
     method2();
     method3();
     method4();
     method5();
 }
 method6();

Если мы собираемся обернуть каждый метод, который может генерировать исключение в блоке try / catch, какой смысл вообще иметь исключения в языковой функции? Там нет никакой пользы, и есть больше печатать.

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

rc = method1();
if( rc != error ) {
     rc = method2();
     if( rc != error ) {
         rc = method3();
         if( rc != error ) {
             rc = method4();
             if(rc != error ) {
                 method5();
             }
         }
     }
 }
 method6();

Это был невероятно распространенный шаблон, и исключения, которые были очень аккуратно решены, позволили вам вернуться к первому примеру выше. Правило стиля, которое говорит, что каждый метод имеет свой собственный блок исключений, полностью выбрасывает это в окно. Зачем вообще беспокоиться?

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

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

void method0() : throws MyNewException 
{
    try {
        method1();  // throws MyOtherException
    }
    catch(e) {
        if(e == MyOtherException)
            throw MyNewException();
    }
    method2();
}

Здесь вы просто добавляете слои, а слои добавляют путаницу. Просто сделай это:

void method0() : throws MyOtherException
{
    method1();
    method2();
}

В дополнение к этому, я чувствую, что если вы следуете этому правилу и обнаружите, что в методе более пары блоков try / catch, вам следует задать вопрос, не является ли сам метод слишком сложным и должен ли быть разбит на несколько методов ,

Вывод: блок try должен содержать набор кода, который должен быть прерван в случае возникновения ошибки в любом месте блока. Неважно, будет ли этот набор кода одной строкой или тысячей строк. (Хотя, если у вас есть тысяча строк в методе, у вас есть другие проблемы.)

Горт Робот
источник
Стивен - спасибо за хорошо выложенный ответ, и я полностью согласен с твоими мыслями. У меня нет проблем с разумно связанным или последовательным кодом типа, живущим в одном блоке try. Если бы я смог опубликовать реальный фрагмент кода, он бы показал 5-10 совершенно не связанных вызовов до первого бросаемого вызова. Один конкретный блок finally оказался намного хуже - было запущено несколько новых потоков и были вызваны дополнительные неочищенные подпрограммы. И в этом отношении все вызовы в этом окончательном блоке не были связаны ни с чем, что могло бы быть брошено.
Одна из основных проблем этого подхода заключается в том, что он не проводит различий между случаями, когда method0может возникать квази-ожидание того, что может быть выдано исключение, и тем, где это не так. Например, предположим, что method1иногда может выдать InvalidArgumentException, но если этого не произойдет, он переведет объект во временно недействительное состояние, которое будет исправлено method2, что, как ожидается, всегда вернет объект в допустимое состояние. Если method2 сгенерирует InvalidArgumentException, код, который ожидает перехват такого исключения method1, перехватит его, даже если ...
supercat
... состояние системы будет соответствовать ожиданиям кода. Если исключение, выданное методом method1, должно обрабатываться иначе, чем исключение, выданное методом method2, как это может быть разумным образом достигнуто без их переноса?
суперкат
В идеале ваши исключения достаточно гранулированы, что является редкой ситуацией.
Gort the Robot