Как объяснить разработчику, что добавление дополнительных if-else, если условия не являются хорошим способом "улучшить" читаемость?

Недавно я столкнулся с следующим кодом на С++:

if (a)
{
  f();
}
else if (b)
{
  f();
}
else if (c)
{
  f();
}

Где a, b и c - все разные условия, и они не очень короткие.

Я попытался изменить код на:

if (a || b || c)
{
  f();
}

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

1) Вы не должны увеличивать читаемость, заменив один ветвящийся оператор тремя (хотя я действительно сомневаюсь, что можно сделать код более читаемым, используя else, если вместо ||).

2) Это не самый быстрый код, и ни один компилятор не будет его оптимизировать.

Но мои аргументы не убедили его.

Что бы вы сказали программисту, пишущему такой код?

Считаете ли вы, что сложное условие является оправданием для использования else if вместо ИЛИ?

Ответ 1

Этот код является избыточным. Он подвержен ошибкам.

Если вы должны были заменить f(); когда-нибудь другим, есть опасность, что вы пропустите один из них.

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

Что касается подхода, объясняющего это вашему коллеге, то это обсуждалось много раз. Смотрите здесь:

Как вы говорите кому-то, написав плохой код?

Как оправдать своим коллегам, что они производят дрянной код?

Как вы обрабатываете код низкого качества от членов команды?

"Наставник" старшего программиста или коллеги без оскорблений

Ответ 2

Замените три сложных условия на одну функцию, сделав очевидным, почему должен выполняться f().

bool ShouldExecute; { return a||b||c};
...
if ShouldExecute {f();};

Ответ 3

Поскольку условия длинные, попросите его сделать это:

if ( (aaaaaaaaaaaaaaaaaaaaaaaaaaaa)
  || (bbbbbbbbbbbbbbbbbbbbbbbbbbbb)
  || (cccccccccccccccccccccccccccc) )
{
    f();
}

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

Ответ 4

В общем, я думаю, что вы правы в том, что if (a || b || c) { f(); } легче читать. Он также мог бы использовать пробелы, чтобы помочь разделить три блока.

Тем не менее, мне было бы интересно посмотреть, как выглядят a, b, c и f. Если f - всего лишь один вызов функции, и каждый блок большой, я могу видеть его точку, хотя я сжимаю при нарушении принципа DRY, вызывая f три разных раза.

Ответ 5

Производительность здесь не проблема.

Многие люди обертываются во флагом "удобочитаемости", когда это действительно просто вопрос индивидуального вкуса.

Иногда я делаю это одним способом, а иногда и другим. Я думаю об этом -

"Какой способ облегчит редактирование тех изменений, которые могут быть сделаны в будущем?"

Не потейте мелкие вещи.

Ответ 6

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

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

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

Ответ 7

Я очень сомневаюсь, что это принесет какие-либо выгоды от производительности, за исключением, по крайней мере, в очень специфическом сценарии. В этом сценарии вы меняете a, b и c, и, следовательно, какой из трех, которые запускают код, изменяется, но код все равно выполняется, тогда уменьшение кода на один if-statement может улучшиться, поскольку у CPU может быть код в кэш ветки, когда он доберется до него в следующий раз. Если вы тройной код, так что он занимает в 3 раза больше места в кэше веток, есть вероятность того, что один или несколько путей будут вытолкнуты, и, таким образом, вы не будете выполнять наиболее эффективное выполнение.

Это очень низкий уровень, поэтому я сомневаюсь, что это сильно повлияет.

В отношении читаемости, которую легче читать:

если что-то, сделайте это
если что-то еще, сделайте это
если еще что-то еще, сделайте это
"this" одинаково во всех трех случаях

или это:

если что-то или что-то еще или еще что-то еще, тогда сделайте это

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

Поддержание работоспособности снижается с помощью 3 организации if-statement из-за этого.

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

В принципе, я бы сказал программисту, что если у него проблемы с чтением 1-х-заявочного способа его написания, возможно, С++ не то, что он должен делать.

Теперь предположим, что части "a", "b" и "c" действительно большие, так что OR в нем теряется во множестве шумов с помощью скобок или нет.

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

bool doExecute = false;
if (a) doExecute = true;
if (b) doExecute = true;
if (c) doExecute = true;
if (doExecute)
{
    f();
}

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

bool doExecute = a;
doExecute = doExecute || b;
doExecute = doExecute || c;
if (doExecute)
{
    f();
}

Ответ 8

Производительность не должна даже ставиться под сомнение.

Возможно, позже он не будет называть f() во всех трех условиях

Ответ 9

Повторяющийся код не делает вещи более ясными, ваш (a || b || c) намного яснее, хотя, возможно, он может реорганизовать его еще больше (начиная с его С++), например.

x.f()