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

Question

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

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

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

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

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

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

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

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

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

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

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

Answer 1

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

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

---

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

---

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

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

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

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

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

Answer 2

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

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

Answer 3

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

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

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

Answer 4

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

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

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

«Каким образом будет проще редактировать виды изменений, которые могут потребоваться в будущем?»

Не парься по мелочам.

Answer 5

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

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

Answer 6

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

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

Что касается читабельности, которую легче читать:

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

или это:

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

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

Из-за этого ремонтопригодность ухудшается с организацией if-3.

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

По сути, я бы сказал программисту, что если у него проблемы с чтением способа написания 1 оператора if, возможно, C ++ - это не то, что он должен делать.

Теперь давайте предположим, что части "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();
}

Answer 7

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

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

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

Answer 8

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

x.f()

Answer 9

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

Может быть, позже он не вызовет f () во всех 3 состояниях