Считаете ли вы эту технику "ПЛОХОЙ"?

Я думаю, что в принципе нет ничего плохого в технике. Я думаю, что я бы позаботился о том, чтобы имя bool было более описательным, чем isGood. Хотя теперь, когда я смотрю на это, почему бы не поместить весь код, который находится в цикле, в один блок if (! IsGood)? Цикл выполняется только один раз.

У меня нет проблем с этим, пока код читабелен.

Вы в значительной степени просто маскируете «goto» под фальшивую петлю. Нравится вам goto или нет, вы бы так же далеко впереди, используя настоящий неприкрытый goto.

Лично я бы просто написал это как

bool isGood = true;

   .... some code

   if (isGood)
   {
   .... some more code
   }

   if (isGood)
   {
   .... some more code
   }

У вас сложный нелинейный поток управления внутри трудно распознаваемой идиомы. Так что да, я считаю эту технику плохой.

Возможно, стоит потратить время на то, чтобы выяснить, можно ли это написать немного лучше.

Зачем использовать фальшивую петлю? Вы можете сделать то же самое с методом, и это, вероятно, не будет считаться «плохой практикой», поскольку это более ожидаемо.

someMethod()
{
   .... some code

   if (!isGood)
       return;

   .... some more code

   if (!isGood)
       return;

   .... some more code

 }

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

public bool Method1(){ ... }
public bool Method2(){ ... }

public void DoStuff(){
    bool everythingWorked = Method1() && Method2();
    ... stuff you want executed always ...
}

Причина, по которой это работает, связана с так называемой логикой короткого замыкания. Method2 не будет вызываться, если Method1 вернет false.

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

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

Почему бы просто не использовать

if (isGood)
{
...Execute more code
}

?

То, что вы пытаетесь сделать, - это нелокальное восстановление после сбоя. Для этого и нужен goto. Используй это. (на самом деле, это то, для чего нужна обработка исключений - но если вы не можете это использовать, лучше всего подойдут 'goto' или 'setjmp / longjmp').

Этот шаблон, шаблон if (succeeded(..)) и «очистка перехода» - все три семантически и структурно эквивалентны. Используйте тот, который наиболее часто встречается в вашем проекте кода. Последовательность очень ценится.

Я бы предостерегал против if (failed(..)) break; в одном моменте, так как вы получите неожиданный результат, если попытаетесь вложить циклы:

do{
   bool isGood = true;
   .... some code
   if (!isGood)
       break;
   .... some more code
   for(....){
      if (!isGood)
          break; // <-- OOPS, this will exit the 'for' loop, which 
                 //  probably isn't what the author intended
      .... some more code
   }
} while(false);
..... some other code, which has to be executed.

Ни у goto cleanup, ни у if (succeeded(..)) нет такого сюрприза, поэтому я бы посоветовал вместо этого использовать один из этих двух.

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

Как насчет использования более привычных языковых функций, таких как функции?

bool success = someSensibleFunctionName();

if (success)
{
   ...
}

someCommonCodeInAnotherFunction();

По сути, вы только что описали goto. Я все время использую goto в C. Я не считаю это плохим, если только вы не используете его для имитации цикла (никогда не делайте этого!). Я обычно использую goto в C для эмуляции исключений (C не имеет исключений):

// Code

if (bad_thing_happened) goto catch;

// More code

if (bad_thing_happened) goto catch;

// Even more code

finally:

// This code is executed in any case
// whether we have an exception or not,
// just like finally statement in other
// languages

return whatever;

catch:

// Code to handle bad error condition

// Make sure code tagged for finally
// is executed in any case
goto finally;

За исключением того факта, что catch и finally имеют противоположный порядок, я не понимаю, почему этот код должен быть ПЛОХОЙ только потому, что он использует goto, если настоящий код try / catch / finally работает точно как это и просто не использует goto. Это бессмысленно. И поэтому я не понимаю, почему ваш код должен быть помечен как ПЛОХОЙ.

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

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

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

Если случай, когда это не «хорошо», действительно является исключительным, то лучшим выбором будет выбрасывание исключений.

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

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

Если разделить код между если (! isGood) перерыв; на отдельные функции, можно получить десятки функций, содержащих всего пару строк, так что исполнители ничего не упрощают. Я не мог использовать возвращаться, потому что я не готов покинуть функцию, я все еще хочу там заняться. Я согласен с тем, что, вероятно, мне следует просто согласиться на отдельное условие if (isGood) {...} для каждой части кода, которую я хочу выполнить, но иногда это приводило к МНОГО фигурных скобок. Но я думаю, я согласен с тем, что людям не очень нравится такая конструкция, поэтому условия для всего ветры!
Спасибо за ваши ответы.

You can use exceptions for that, but exceptions generally are not recommended in normal application logic, only for abnormal situations.

Нет ничего плохого в использовании исключений. Исключения являются частью логики приложения. Рекомендация относительно исключений заключается в том, что они должны быть относительно редкими во время выполнения.

Для меня то, что вы делаете, во многих отношениях плохо. Цикл можно заменить, поместив этот код в метод.

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

If (isError)
{
   //Do whatever you need to do for the error and
   return;
}

If (!isGood)
{
  //Do something
}

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

Одна из первых вещей, над которыми я работал 28 лет назад, была программа на Фортране, которая всегда должна была проверять, доступно ли графическое устройство. Кто-то принял важное решение вызвать логическое значение для этого LNOGRAF, поэтому, если бы было доступно графическое устройство, это было бы ложью. Я считаю, что это было настроено так из-за ложной эффективности, проверка устройства вернула ноль, если было графическое устройство. На протяжении всего кода все проверки заключались в том, чтобы увидеть, доступно ли графическое устройство. Он был полон:

Если (.NOT. LNOGRAF)

Не думаю, что был сингл:

Если (LNOGRAF)

в программе. Это использовалось в программном обеспечении планирования миссий для B-52 и крылатых ракет. Это определенно научило меня называть свои переменные по тому, что я действительно проверяю.

Это запутанно и запутанно, я бы сразу отказался от этого.

Рассмотрим эту альтернативу:

private void DoSomething()
{
   // some code
   if (some condition)
   {
      return;
   }
   // some more code
   if (some other condition)
   {
      return;
   }
   // yet more code
}

Также рассмотрите возможность разбиения приведенного выше кода на несколько методов.

Я использую эту идиому много лет. Я считаю, что это предпочтительнее аналогичных вложенных или последовательных if, «goto onError» или многократных возвратов. Приведенный выше пример с вложенным циклом - это то, чего стоит остерегаться. Я всегда добавляю комментарий к слову «делать», чтобы было понятно всем, кто не знаком с этой идиомой.

А как насчет функционального подхода?

        void method1()
        {
            ... some code
            if ( condition )
                method2();
        }

        void method2()
        {
            ... some more code
            if ( condition )
                method3();
        }

        void method3()
        {
            ... yet more code
            if ( condition )
                method4();
        }

Плохая практика, зависит от обстоятельств.

В этом коде я вижу очень творческий способ написать goto с менее пахнущими серой ключевыми словами.

У этого кода есть несколько альтернатив, которые могут быть или не могут быть лучше, в зависимости от ситуации.

Ваше решение "делать / пока"

Ваше решение интересно, если у вас много кода, но вы оцените "выход" из этой обработки в некоторых ограниченных точках:

do
{
   bool isError = false ;

   /* some code, perhaps setting isError to true */
   if (isError) break ;
   /* some code, perhaps setting isError to true */
   if (isError) break ;
   /* some code, perhaps setting isError to true */
}
while(false) ;
   
// some other code   

Проблема в том, что вы не можете легко использовать ваш «if (isError) break;» это цикл, потому что он выйдет только из внутреннего цикла, а не из вашего блока do / while.

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

Я не буду обсуждать альтернативы, использующие «если» или даже вложенные «если», потому что, немного подумав, я нахожу их решения вашей проблемы хуже, чем ваши собственные.

Вызов goto a ... goto

Возможно, вам следует четко изложить тот факт, что вы используете goto, и задокументировать причины, по которым вы выбрали это решение по сравнению с другим.

По крайней мере, он покажет, что с кодом что-то не так, и предложит рецензентам подтвердить или признать недействительным ваше решение.

Вы все равно должны открыть блок и вместо его разрыва использовать goto.

{
   // etc.
   if (/*some failure condition*/) goto MY_EXIT ;
   // etc.

   while(/* etc.*/)
   {
      // etc.
      for(/* etc.*/)
      {
         // etc.
         if (/*some failure condition*/) goto MY_EXIT ;
         // etc.
      }
      // etc.
      if (/*some failure condition*/) goto MY_EXIT ;
      // etc.
   }

   // etc.
}

MY_EXIT:
   
// some other code   

Таким образом, когда вы выходите из блока через goto, у вас нет возможности обойти некоторый конструктор объекта с помощью goto (что запрещено C++).

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

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

попробуй поймать

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

try
{
   // All your code
   // You can throw the moment something fails
   // Note that you can call functions, use reccursion,
   // have multiple loops, etc. it won't change
   // anything: If you want to exit the process,
   // then throw a MyExitProcessException exception.

   if (/* etc. */)
   {
      // etc.
      while(/* etc.*/)
      {
         // etc.
         for(/* etc.*/)
         {
            // etc.
            if (/*some failure condition*/) throw MyExitProcessException() ;
            // etc.
         }
         // etc.

         callSomeFunction() ;
         // the function will throw if the condition is met
         // so no need to test a return code

         // etc.
      }
      // etc.
   }

   // etc.
}
catch(const MyExitProcessException & e)
{
   // To avoid catching other exceptions, you should
   // define a "MyExitProcessException" exception
}

// some other code

Если какое-либо условие в приведенном выше коде или внутри некоторых функций, вызываемых указанным выше кодом, не выполняется, генерируется исключение.

Это несколько тяжелее, чем ваше решение do / while, но имеет те же преимущества и может даже прервать обработку из внутренних циклов или из внутренних вызываемых функций.

Обсуждение

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

Предположим, что.

Если вы можете закодировать это с помощью try / catch, сделайте это: Чтобы прервать сложный фрагмент кода, выбрасывание исключения - правильное решение (не следует недооценивать тот факт, что вы можете добавить информацию об ошибке / успехе в свой объект исключения). После этого у вас будет более четкий код.

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

Никто не может отрицать, что ваше решение - это прославленный goto. Не будет кода goto-spaghetti, потому что do / while не позволит вам это сделать, но это все еще семантический goto. Это может быть причиной того, что некоторые могут посчитать этот код "плохим": они нюхают goto, не находя четко его ключевое слово.

Только в этом случае (и в этом случае с профилированной проверкой производительности) ваше решение кажется нормальным и лучше, чем альтернатива с использованием if), но с меньшим качеством (IMHO), чем решение goto, которое, по крайней мере, не скрывает себя за ложной петлей.

Заключение

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

Итак, в порядке предпочтения:

1. Используйте try / catch
2. Используйте goto
3. Используйте цикл do / while
4. Используйте ifs / вложенные ifs

Хорошо это или плохо, но я использовал эту конструкцию в нескольких местах. Однако начало этого четко задокументировано:

    /* This is a one-cycle loop that simplifies error handling */
    do
    {
        ...modestly complex code, including a nested loop...
    } while (0);

Это на C, а не на C++, поэтому исключения не возможны. Если бы я использовал C++, я бы серьезно подумал об использовании исключений для обработки исключений. Идиома повторного тестирования, предложенная Джереми, также разумна; Я использовал это чаще. RAII мне тоже очень поможет; к сожалению, C не поддерживает это легко. И использование большего количества функций могло бы помочь. Обработка разрывов внутри вложенного цикла выполняется повторным тестом.

Я бы не назвал это отличным стилем; Я бы не стал автоматически классифицировать его как "ПЛОХОЙ".

Я думаю, что здесь люди нечестны.

Если бы я, как руководитель вашей группы, увидел бы такой код, вы были бы готовы пообщаться один на один и пометили бы его как потенциальную проблему для команды, поскольку этот фрагмент кода особенно ужасен.

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

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

Решение Пола нельзя использовать, когда в вашем цикле есть дорогостоящие инициализации объекта. Если созданные объекты используются на более поздних этапах, решение do while (0) лучше.

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

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

Другие предположили, что это трудная для понимания идиома. Что ж, сначала вы можете поместить комментарий, как предлагается, в верхней части цикла. Во-вторых, do-while (0) - это нормальная и распространенная идиома в макросах, которую все программисты на C должны сразу распознать, поэтому я просто не верю на это.

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

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

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

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

if (condition1(...) && condition2(...) && condition3(...) && ... && conditionN(...)) {
    // code that ought to run after all conditions
};
// code that ought to run whether all conditions are met or not

Тогда я думаю, что это было бы более "читабельным" и более идиоматичным. Таким образом, вы можете создавать такие функции, как:

bool conditionN(...) {
    if (!real_condition) return false;
    // code that ought to run
    return true;
};

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

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

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

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