Инвертирование оператора IF

39

Так что я программирую уже несколько лет, а недавно начал больше использовать ReSharper. Одна вещь, которую ReSharper всегда предлагает мне, - это «инвертировать» if, чтобы уменьшить вложенность ».

Допустим, у меня есть этот код:

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
    }
}

И ReSharper предложит мне сделать это:

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;
}

Кажется, что ReSharper ВСЕГДА предложит мне инвертировать IF, независимо от того, сколько вложений происходит. Эта проблема в том, что я люблю вложенность, по крайней мере, в НЕКОТОРЫХ ситуациях. Мне кажется, легче читать и выяснять, что происходит в определенных местах. Это не всегда так, но иногда я чувствую себя более комфортно.

У меня вопрос: кроме личных предпочтений или читабельности, есть ли причина уменьшить вложенность? Есть ли разница в производительности или что-то еще, о чем я могу не знать?

Барри Франклин
источник
1
Возможный дубликат Контролируйте уровень отступа минимума
комар
24
Ключевым моментом здесь является «Решарпер предлагает». Взгляните на предложение, если оно облегчает чтение кода и является последовательным, используйте его. Это делает то же самое с for / for каждого цикла.
Джон Рейнор
Я обычно опускаю эти резкие подсказки, я не всегда следую, поэтому они больше не появляются в боковой панели.
CodesInChaos
1
ReSharper удалил негатив, что обычно хорошо. Тем не менее, он не предлагал троичное условие, которое могло бы уместиться здесь, если блок действительно такой простой, как ваш пример.
dcorking
2
Разве ReSharper также не предлагает переместить условное выражение в запрос? foreach (someObject в someObjectList.Where (object => object! = null)) {...}
Роман Райнер,

Ответы:

63

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

Мы всего лишь люди, и одновременно держать в голове множество вещей сложно.

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

Энди
источник
64
Я люблю видеть, как поток мнения разработчиков идет таким путем. В коде так много вложений просто потому, что существует необычайно популярное заблуждение о том break, что continueи множество returnне структурировано.
JimmyJames
6
проблема заключается в прерывании и т. д. - это все еще поток управления, который вы должны держать в своей голове (это не может быть x, иначе он вышел бы из цикла сверху), возможно, не нужно больше думать, если это нулевая проверка, которая в любом случае вызвала бы исключение. Но это не так хорошо, когда его более нюансированный «запускайте только половину этого кода для этого статуса по четвергам»
Ewan
2
@Ewan: Какая разница между «это не может быть х, иначе он вышел бы из цикла сверху» и «это не может быть х, иначе он не вошел бы в этот блок»?
Коллин
ничто не является сложностью и значением х, что важно
Эван
4
@ Иван: Я думаю, что break/ continue/ returnимеют реальное преимущество перед elseблоком. При раннем выходе есть 2 блока : блок выхода и блок пребывания там; с elseесть 3 блока : if, else и все, что будет после (которое используется независимо от выбранной ветви). Я предпочитаю иметь меньше блоков.
Матье М.
33

Не избегайте негативов. Люди сосут, разбирая их, даже когда процессор их любит.

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

foo != nullК сожалению, стал идиомой. Я едва замечаю отдельные части больше. Я смотрю на это и просто думаю: «О, сейчас уже безопасно foo».

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

Однако то, что вы нам дали, было так:

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;
}

Что даже не является структурно одинаковым!

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;

    if(someGlobalObject == null) continue;
    someSideEffectObject = someGlobalObject.SomeProperty;
}

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

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

candied_orange
источник
1
Что ж, если у вас есть больше кода в цикле, то, как он работает, зависит от того, как все это объединится. Это не независимый код. Реорганизованный код в вопросе был верным для примера, но, возможно, не верен, когда не изолирован - разве это был смысл? (+1 для не избегайте негативов, хотя)
Baldrickk
@Baldrickk if (foo != null){ foo.something() }позволяет мне добавлять код без заботы, если он fooбыл нулевым. Это не Даже если мне не нужно делать это сейчас, я не чувствую мотивации гадать на будущее, говоря: «Ну, они могут просто изменить его, если им это нужно». Фэ. Программное обеспечение мягкое, только если его легко изменить.
candied_orange
4
«продолжайте добавлять код, не заботясь». Либо код должен быть связан (в противном случае он, вероятно, должен быть отдельным), либо следует позаботиться о том, чтобы понять функциональность изменяемой функции / блока, поскольку добавление любого кода может изменить поведение. за то, что предполагалось. Для простых случаев, подобных этому, я не думаю, что это так или иначе имеет значение. В более сложных случаях я ожидал бы, что понимание кода станет более точным, поэтому я бы выбрал тот, который мне кажется наиболее понятным, что может быть любым стилем.
Baldrickk
связанные и переплетенные не одно и то же.
candied_orange
1
Не избегайте негативов: D
VitalyB
20

Это старый аргумент о том, хуже ли разрыв, чем гнездование.

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

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

foreach (someObject in someObjectList.where(i=>i != null))
{
    someOtherObject = someObject.SomeProperty;
}

Очевидно, что вложение делает сложным для понимания, когда у вас много слоев

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
        if(someObject.x > 5) 
        {
            x ++;
            if(someObject.y > 5) 
            {
                x ++;
            }
        }
    }
}

Хуже всего, когда у вас есть оба, хотя

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
        if(someObject.x > 5) 
        {
            x ++;
            if(someObject.y > 5) 
            {
                x ++;
                return;
            }
            continue;
        }
        someObject.z --;
        if(myFunctionWithSideEffects(someObject) > 6) break;
    }
}
Ewan
источник
11
Люблю эту строчку: foreach (subObject in someObjectList.where(i=>i != null))не знаю, почему я никогда не думал о чем-то подобном.
Барри Франклин
@BarryFranklin ReSharper должен подчеркивать зеленую пунктирную линию подчеркивания на foreach с «Конвертировать в выражение LINQ» в качестве предложения, которое бы сделало это для вас. В зависимости от операции он может также выполнять другие методы linq, такие как Sum или Max.
Кевин Фи
@BarryFranklin Это, вероятно, предложение, которое вы хотите установить на низком уровне и использовать только по своему усмотрению. Хотя он отлично работает для таких простых случаев, как этот пример, я обнаружил, что в более сложном коде он выбирает более сложные условные части в биты, которые Linqifiable, а оставшееся тело имеет тенденцию создавать вещи, которые гораздо сложнее понять, чем оригинал. Обычно я, по крайней мере, попробую это предложение, потому что, когда он может преобразовать весь цикл в Linq, результаты часто являются разумными; но половина и половина результатов, как правило, получаются уродливыми быстро IMO.
Дэн Нили
По иронии судьбы, редактирование с помощью resharper имеет точно такой же уровень вложенности, потому что оно также имеет if, за которым следует одна строка.
Питер
хех, без брекетов! что, по-видимому, разрешено: /
Эван
6

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

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;

    ...  imagine that there is some code here ...

    // added later by a Junior Developer
    doSomeOtherFunction(someObject);
}

Вы хотите вызвать doSomeOtherFunction()для всех someObject или только если не ноль? С оригинальным кодом у вас есть возможность, и она понятнее. С continueодним из них можно забыть «о, да, там мы пропустили нули», и у вас даже не будет возможности вызвать его для всех объектов someObject, если вы не поместите этот вызов в начало метода, который может оказаться невозможным или правильным по другим причинам.

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

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

user949300
источник
12
В цикле не должно быть достаточно кода, чтобы вы не могли видеть нулевую проверку сверху без прокрутки.
Брайан
@ Брайан Бетчер согласился, но не весь код написан так хорошо. И даже несколько строк сложного кода могут заставить забыть о (или ошибиться) продолжении. На практике я в порядке, returnпотому что они образуют «чистый разрыв», но IMO breakи continueприводят к путанице, и в большинстве случаев их лучше избегать.
user949300
4
@ user949300 После десяти лет разработки я никогда не находил перерыв и не по-прежнему вызываю путаницу. Я вовлек их в замешательство, но они никогда не были причиной. Обычно плохие решения в отношении разработчика были причиной, и они вызывали бы путаницу, независимо от того, какое оружие они использовали.
CorsiKa
1
@corsiKa Ошибка Apple SSL - на самом деле ошибка goto, но она могла легко быть ошибкой перерыва или продолжения. Однако код ужасен ...
user949300
3
@BryanBoettcher Мне кажется, что проблема в том, что те же разработчики, которые думают, что продолжение или множественные возвраты в порядке, также думают, что хоронить их в больших телах методов тоже можно. Так что каждый мой опыт с continue / многократным возвратом был в огромных путях кода.
Энди
3

Идея заключается в скорейшем возвращении. Ранний returnцикл становится ранним continue.

Много хороших ответов на этот вопрос: должен ли я вернуться из функции раньше или использовать оператор if?

Обратите внимание, что хотя вопрос закрыт как основанный на мнении, более 430 голосов проголосовали за досрочное возвращение, ~ 50 голосов «зависит» и 8 - против досрочного. Таким образом, существует исключительно сильный консенсус (либо это, либо я плохо считаю, что вполне вероятно).

Лично, если тело цикла будет подвергаться раннему продолжению и достаточно долго, чтобы оно имело значение (3-4 строки), я стремлюсь извлечь тело цикла в функцию, где я использую ранний возврат вместо раннего продолжения.

Питер
источник
2

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

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

Вот игрушечный пример того, что можно выкатить

function foobar(x, y, z) {
    if (x > 0) {
        if (z != null && isGamma(y)) {
            // ~10 lines of code
            // return something
        }
        else {
           return y
        }
    }
    else {
      return y + z
    }
}

Код подобный этому, где есть ОДИН интересный случай (где остальные просто возвращаются или небольшие блоки операторов) являются общими. Тривиально переписать вышесказанное как

function foobar(x, y, z) {
    if (x <=0) {
        return y + z
    }
    if (z == null || !isGamma(y) {
       return y
    }
    // ~ 10 lines of code
    // return something
}

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


В ответ на пример кода OP я согласен с тем, что это чрезмерный коллапс. Тем более что в 1TB, стиль, который я использую, инверсия приводит к увеличению размера кода.

ЛВС
источник
0

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

При прочих равных условиях предпочтительнее меньше вложенности, поэтому ReSharper предложит преобразование, уменьшающее вложенность. Но все остальные вещи не равны: в этом случае преобразование требует введения a continue, а это означает, что на самом деле сложный код будет сложнее следовать.

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

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

JacquesB
источник
0

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

for(obj in objectList) {
    if(obj == null) continue;
    if(obj.getColour().equals(Color.BLUE)) {
        // Handle blue objects
    } else {
        // Handle non-blue objects
    }
}

Обратите внимание, что в Java Streams или других функциональных идиомах вы можете отделить фильтрацию от зацикливания, используя:

items.stream()
    .filter(i -> (i != null))
    .filter(i -> i.getColour().equals(Color.BLUE))
    .forEach(i -> {
        // Process blue objects
    });

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

foreach my $item (@items) {
    next unless defined $item;
    carp "Only blue items are supported! Failed on item $item" 
       unless ($item->get_colour() eq 'blue');
    ...
}
Gaurav
источник