Это плохая практика, чтобы изменить состояние внутри оператора if?

Question

Я написал код, похожий на следующий:

String SKIP_FIRST = "foo";
String SKIP_SECOND = "foo/bar";

int skipFooBarIndex(String[] list){
    int index;
    if (list.length >= (index = 1) && list[0].equals(SKIP_FIRST) ||
        list.length >= (index = 2) && 
        (list[0] + "/" + list[1]).equals(SKIP_SECOND)){
        return index;
    }

    return 0;
}

String[] myArray = "foo/bar/apples/peaches/cherries".split("/");
print(skipFooBarIndex(myArray);

Это изменяет состояние внутри оператора if путем присвоения индекса. Однако моим коллегам это очень не понравилось.

Это вредная практика? Есть ли причина для этого?

Answer 1

Да. Это явно снижает читабельность. Что не так со следующим кодом?

int skipFooBarIndex(String[] list){
    if(list.length >= 1 && list[0].equals(SKIP_FIRST)) 
        return 1;
    if(list.length >= 2 && (list[0] + "/" + list[1]).equals(SKIP_SECOND))
        return 2;
    return 0;
}

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

Предполагая, что вы считаете его "умным" кодом, хорошо всегда помнить цитату Брайана Кернигана:

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

Answer 2

... Однако моим коллегам это очень не понравилось ...

Да, это так. Не только потому, что вы можете так его кодировать, вы должны.

Помните, что этот кусок кода в конечном итоге кто-то должен будет поддерживать (что кто-то может стать вами самим через 8 месяцев)

Изменение состояния внутри if, make труднее читать и понимать (в основном из-за того, что оно не распространено)

Цитата Мартина Фаулера:

Любой дурак может написать код, понятный компьютеру. Хорошие программисты пишут код, понятный людям

Answer 3

Проблема в том, что код будет генерировать несколько WTF в сеансе проверки кода. Что-нибудь, что заставляет людей идти "ждать, что?" должен идти.

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

Answer 4

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

Answer 5

Это вредная практика?

Абсолютно да. Код трудно понять. Это занимает два или три чтения для всех, кроме автора. Любой код, который трудно понять и который может быть переписан более простым и легким для понимания способом, ДОЛЖЕН быть переписан таким образом.

Ваши коллеги абсолютно правы.

Есть ли причина для этого?

Единственная возможная причина для того, чтобы сделать что-то подобное, заключается в том, что вы подробно профилировали приложение и обнаружили, что эта часть кода является существенным узким местом. Затем вы реализовали мерзость, описанную выше, повторно запустили профилировщик и обнаружили, что он ДЕЙСТВИТЕЛЬНО улучшает производительность.

Answer 6

Заимствовано из cppreference.com :

Одним из важных аспектов C ++, связанных с приоритетом операторов, является порядок вычисления и порядок побочных эффектов в выражениях. В некоторых случаях порядок, в котором происходят вещи, не определен. Например, рассмотрим следующий код:

float x = 1;
x = x / ++x;

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

Кроме того, хотя ++ x оценивается как x + 1, побочный эффект фактического сохранения этого нового значения в x может произойти в разное время, что приведет к различным значениям для x.

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

Answer 7

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

if (list.length >= 1 && list[0].equals(SKIP_FIRST)) {
    return 1;
}

if (list.length >= 2 && (list[0] + "/" + list[1]).equals(SKIP_SECOND)) {
    return 2;
}

Answer 8

Да, при просмотре кода трудно проследить побочные эффекты.

Относительно причин сделать это: Нет, нет никакой реальной причины сделать это. Я еще не наткнулся на утверждение if, которое нельзя переписать без побочных эффектов без каких-либо потерь.

Answer 9

Говоря как кто-то, кто много занимается программированием обслуживания: если бы я столкнулся с этим, я бы проклял вас, заплакал, а затем изменил бы это.

Код, похожий на этот, - кошмар - кричит одна из двух вещей

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

;)

Answer 10

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

int skipFooBarIndex(String[] list) {
    return (list.length > 0 && list[0].equals(SKIP_FIRST)) ? 1 :
       ((list.length > 1 && (list[0] + "/" + list[1]).equals(SKIP_SECOND)) ? 2 : 0);
}

Хотя этот пример менее читабелен.