Оценка короткого замыкания, это плохая практика?

88

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

if (smartphone != null && smartphone.GetSignal() > 50)
{
   // Do stuff
}

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

Это может быть полезно и в других ситуациях, таких как проверка того, что индекс находится в границах массива, и, насколько мне известно, такая техника может выполняться на разных языках: Java, C #, C ++, Python и Matlab.

Мой вопрос: является ли этот вид кода представлением плохой практики? Эта плохая практика возникает из-за какой-то скрытой технической проблемы (то есть это может в конечном итоге привести к ошибке), или это приводит к проблеме читаемости для других программистов? Может ли это быть запутанным?

Innova
источник
69
Технический термин - оценка короткого замыкания . Это хорошо известный метод реализации, и если языковой стандарт гарантирует это, полагаться на него - это хорошо. (Если этого не произойдет, это не так уж много языка, IMO.)
Килиан Фот
3
Большинство языков позволяют вам выбирать, какое поведение вы хотите. В C # оператор ||короткого замыкания, оператор |(один канал) не делает ( &&и &ведет себя одинаково). Поскольку операторы короткого замыкания используются гораздо чаще, я рекомендую помечать использование не коротких замыканий комментарием, чтобы объяснить, почему вам нужно их поведение (в основном такие вещи, как вызовы методов, которые должны выполняться).
linac
14
@linac теперь помещает что-то с правой стороны &или |чтобы убедиться, что побочный эффект случается, это то, что я бы сказал, конечно, плохая практика: вам не будет ничего стоить, чтобы поместить этот метод в отдельную строку.
Джон Ханна
14
@linac: В C и C ++ &&это короткое замыкание и &нет, но они очень разные операторы. &&логическое «а»; &является побитовым "и". Это возможно, x && yчтобы быть правдой, в то время x & yкак ложь.
Кит Томпсон
3
Ваш конкретный пример может быть плохой практикой и по другой причине: что , если это является нулевым? Разумно ли для него быть здесь нулевым? Почему это ноль? Должна ли выполняться оставшаяся часть функции вне этого блока, если она нулевая, все это ничего не должно делать или ваша программа остановится? Распечатка «если ноль» в каждом доступе (возможно, из-за исключительной ссылки, от которой вы спешили избавиться) означает, что вы можете продвинуться довольно далеко в остальной части программы, даже не заметив, что инициализация объекта телефона облажалась вверх. И в конце концов, когда вы, наконец, получите немного кода, которого нет
Random832

Ответы:

125

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

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

Несколько предостережений:

Используйте здравый смысл в отношении сложности и читабельности

Следующее не очень хорошая идея:

if ((text_string != null && replace(text_string,"$")) || (text_string = get_new_string()) != null)

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

Если вам нужно обрабатывать ошибки, часто есть лучший способ

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

Избегайте повторных проверок одного и того же состояния

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

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

Deduplicator
источник
12
Следует отметить, что повторение, если блокирует проверку, является ли экземпляр нулевым при оценке короткого замыкания, вероятно, следует переписать, чтобы выполнить эту проверку только один раз.
Нейл
1
@ Нейл, хорошая мысль; Я обновил ответ.
2
Единственное, что я могу добавить, - это отметить еще один необязательный способ его обработки, который действительно предпочтителен, если вы собираетесь проверять что-то в нескольких различных условиях: проверьте, не является ли что-то пустым в if (), затем возврат / бросок - иначе просто продолжайте. Это исключает вложение и часто может сделать код более явным и лаконичным, как, например, «это не должно быть нулевым, а если это так, то я ухожу отсюда» - размещено как можно раньше в коде, насколько это разумно возможно. Отлично, когда вы пишете код, который проверяет определенное условие более чем в одном месте метода.
BrianH
1
@ dan1111 Я бы обобщил приведенный вами пример следующим образом: «Условие не должно содержать побочный эффект (например, присвоение переменной выше). Оценка короткого замыкания не меняет этого и не должна использоваться в качестве сокращения для бокового эффект, который так же легко можно было бы поместить в тело if, чтобы лучше представить отношение if / then. "
AaronLS
@AaronLS, я категорически не согласен с утверждением, что «условие не должно содержать побочный эффект». Условие не должно быть излишне запутывающим, но пока это чистый код, я в порядке с побочным эффектом (или даже более чем одним побочным эффектом) в условии.
26

Допустим, вы использовали язык C-стиля без &&и нуждались в создании эквивалентного кода, как в вашем вопросе.

Ваш код будет:

if(smartphone != null)
{
  if(smartphone.GetSignal() > 50)
  {
    // Do stuff
  }
}

Эта модель будет много.

Теперь представьте, что версия 2.0 нашего гипотетического языка вводит &&. Подумай, как круто это было бы!

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

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

Нет, вы умны, потому что вы знали, что если первое утверждение было ложным, то нет смысла даже оценивать второе утверждение. Язык туп, как скала, и сделал то, что вы сказали, чтобы сделать. (Джон Маккарти был еще умнее и понял, что оценка короткого замыкания будет полезна для языков).

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

Рассмотреть возможность:

if(smartphone != null && smartphone.GetSignal() > ((range = (range != null ? range : GetRange()) != null && range.Valid ? range.MinSignal : 50)

Это расширяет ваш код для проверки range. Если значение rangeравно NULL, оно пытается установить его с помощью вызова, GetRange()хотя это может привести к сбою, поэтому rangeвсе равно может быть NULL. Если диапазон не равен нулю после этого, и Validтогда используется его MinSignalсвойство, в противном случае используется значение по умолчанию 50.

Это &&тоже зависит от того , но поместить это в одну строку, вероятно, слишком умно (я не уверен на 100%, что я правильно понял, и я не собираюсь перепроверять, потому что этот факт демонстрирует мою точку зрения).

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

Также:

if(smartphone != null && (smartphone.GetSignal() == 2 || smartphone.GetSignal() == 5 || smartphone.GetSignal() == 8 || smartPhone.GetSignal() == 34))
{
  // do something
}

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

if(smartphone != null)
{
  switch (smartphone.GetSignal())
  {
    case 2: case 5: case 8: case 34:
      // Do something
      break;
  }
}

Я бы выиграл как в удобочитаемости, так и в производительности (скорее всего, несколько вызовов GetSignal()не были бы оптимизированы).

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

Последний случай, который отходит от лучших практик:

if(a && b)
{
  //do something
}

По сравнению с:

if(a & b)
{
  //do something
}

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

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

Первый, хотя и имеет еще одну ветку. Подумайте, перепишем ли мы это на нашем гипотетическом языке C-стиля без &&:

if(a)
{
  if(b)
  {
    // Do something
  }
}

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

По этой причине if(a & b)код может быть более эффективным в некоторых случаях.

Здесь я бы сказал, что if(a && b)по-прежнему это лучший практический подход для начала: более распространенный, единственный жизнеспособный в некоторых случаях (где bбудет ошибка, если aложь) и более быстрый, чем нет. Стоит отметить, что if(a & b)в некоторых случаях это часто полезная оптимизация.

Джон Ханна
источник
1
Если у вас есть tryметоды, которые возвращаются trueв случае успеха, о чем вы думаете if (TryThis || TryThat) { success } else { failure }? Это менее распространенная идиома, но я не знаю, как сделать то же самое без дублирования кода или добавления флага. Хотя память, необходимая для флага, не будет проблемой, с точки зрения анализа, добавление флага эффективно разбивает код на две параллельные юниверсы - одна содержит операторы, которые достижимы, когда флаг установлен, trueи другая - операторы, достижимые, когда он есть false. Иногда хорошо с СУХОЙ точки зрения, но неприглядно.
суперкат
5
Я не вижу ничего плохого в if(TryThis || TryThat).
Джон Ханна
3
Чертовски хороший ответ, сэр.
епископ
FWIW, отличные программисты, включая Kernighan, Plauger & Pike из Bell Labs, для ясности рекомендуют использовать мелкие управляющие структуры, например, if {} else if {} else if {} else {}а не вложенные управляющие структуры if { if {} else {} } else {}, даже если это означает оценку одного и того же выражения более одного раза. Линус Торвальдс сказал нечто подобное: «если вам нужно более 3 уровней отступа, вы все равно облажались». Давайте возьмем это как голосование за операторов короткого замыкания.
Сэм Уоткинс
оператор if (a && b); достаточно легко заменить. if (a && b && c && d) Statement1; еще заявление2; это настоящая боль.
gnasher729
10

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

open($handle, "<", "filename.txt") or die "Couldn't open file!";

идиоматично. openвозвращает что-то ненулевое в случае успеха (так что dieчасть после orникогда не происходит), но неопределенное в случае неудачи, и затем происходит вызов die(это способ Perl вызвать исключение).

Это хорошо в языках, где все это делают.

RemcoGerlich
источник
17
Наибольший вклад Perl в языковой дизайн - предоставить множество примеров того, как этого не делать.
Джек Эйдли,
@JackAidley: Я скучаю по Perl unlessи постфиксным условным выражениям ( send_mail($address) if defined $address).
RemcoGerlich
6
@JackAidley, не могли бы вы указать, почему «или умереть» плохо? Это просто наивный способ обработки исключений?
большие камни
В Perl можно сделать много ужасных вещей, но я не вижу проблемы с этим примером. Это просто, кратко и понятно - именно то, что вам нужно от обработки ошибок на языке сценариев.
1
@RemcoGerlich Руби унаследовал некоторые из этого стиля:send_mail(address) if address
Бон
6

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

Проблема в том, что оценка короткого замыкания не является атомарной. Ваш поток может проверить , что smartphoneесть null, другой поток может прийти и свести на нет его, а затем ваш поток быстро пытается GetSignal()и взрывает.

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

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

Telastyn
источник
3

Есть много ситуаций, когда я хочу сначала проверить одно условие, и хочу проверить только второе условие, если первое условие выполнено успешно. Иногда просто для эффективности (потому что нет смысла проверять второе условие, если первое уже не выполнено), иногда потому, что в противном случае моя программа потерпит крах (сначала проверяется NULL), иногда потому, что в противном случае результаты будут ужасными. (ЕСЛИ (я хочу напечатать документ) И (печать документа не удалась)), ТО отобразите сообщение об ошибке - вы не хотите печатать документ и проверить, не удалось ли распечатать документ, если вы не хотите печатать документ в первое место!

Поэтому возникла ОГРОМНАЯ потребность в гарантированной оценке логических выражений при коротком замыкании. Это не присутствовало в Паскале (у которого была негарантированная оценка короткого замыкания), который был главной болью; Я впервые увидел это в Аде и С.

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

gnasher729
источник
«Перепишите его так, чтобы он работал нормально без оценки короткого замыкания, и вернитесь и расскажите нам, как вам понравилось». - Да, это возвращает воспоминания о Паскале. И нет, мне это не понравилось.
Томас Падрон-Маккарти
2

Одно соображение, не упомянутое в других ответах: иногда наличие этих проверок может указывать на возможный рефакторинг в шаблон проектирования Null Object . Например:

if (currentUser && currentUser.isAdministrator()) 
  doSomething();

Можно упростить просто:

if (currentUser.isAdministrator())
  doSomething ();

По currentUserумолчанию используется какое-либо «анонимный пользователь» или «нулевой пользователь» с резервной реализацией, если пользователь не вошел в систему.

Не всегда запах кода, но что-то нужно учитывать.

Пит
источник
-4

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

т.е.

 if(quickLogicA && slowLogicB) {doSomething();}

это хорошо, но

if(logicA && doSomething()) {andAlsoDoSomethingElse();}

это плохо, и, конечно, никто не согласится с ?!

if(doSomething() || somethingElseIfItFailed() && anotherThingIfEitherWorked() & andAlwaysThisThing())
{/* do nothing */}

Рассуждение:

Ваш код ошибки, если обе стороны оцениваются, а не просто не выполняют логику. Утверждалось, что это не проблема, оператор 'и' определен в c #, поэтому смысл понятен. Тем не менее, я утверждаю, что это не так.

Причина 1. Историческая

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

Хотя в c # спецификация языка гарантирует, что логический оператор 'и' делает короткое замыкание. Это не относится ко всем языкам и компиляторам.

Википеда перечисляет различные языки и их краткие замыкания http://en.wikipedia.org/wiki/Short-circuit_evaluation, поскольку вы можете использовать множество подходов. И пару лишних проблем, которые я здесь не обсуждаю.

В частности, FORTRAN, Pascal и Delphi имеют флаги компилятора, которые переключают это поведение.

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

Причина 2. Языковые различия

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

В частности, оператор «и» в VB.Net не замыкает накоротко, а TSQL имеет некоторые особенности.

Учитывая, что современное «решение» может включать в себя несколько языков, таких как javascript, regex, xpath и т. Д., Вы должны принять это во внимание при прояснении функциональности вашего кода.

Причина 3. Различия в языке

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

Причина 4. Ваш конкретный пример нулевой проверки параметра функции

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

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

Тем не менее, это не лучшая практика по нескольким причинам!

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

    if (smartphone == null)
    {
        //throw an exception
    }
    // perform functionality
    
  2. Вы вызываете функцию из внутри условного оператора. Хотя название вашей функции подразумевает, что она просто вычисляет значение, она может скрывать побочные эффекты. например

    Smartphone.GetSignal() { this.signal++; return this.signal; }
    
    else if (smartphone.GetSignal() < 1)
    {
        // Do stuff 
    }
    else if (smartphone.GetSignal() < 2)
    {
        // Do stuff 
    }
    

    Лучшая практика предполагает:

    var s = smartphone.GetSignal();
    if(s < 50)
    {
        //do something
    }
    

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

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

встроенные условия

var s = smartphone!=null ? smartphone.GetSignal() : 0;

и нулевое слияние

var s = smartphoneConnectionStatus ?? "No Connection";

которые обеспечивают аналогичную функциональность короткого кода с большей ясностью

многочисленные ссылки для поддержки всех моих точек зрения:

https://stackoverflow.com/questions/1158614/what-is-the-best-practice-in-case-one-argument-is-null

Проверка параметров конструктора в C # - Лучшие практики

Стоит ли проверять на ноль, если он не ожидает ноль?

https://msdn.microsoft.com/en-us/library/seyhszts%28v=vs.110%29.aspx

https://stackoverflow.com/questions/1445867/why-would-a-language-not-use-short-circuit-evaluation

http://orcharddojo.net/orchard-resources/Library/DevelopmentGuidelines/BestPractices/CSharp

примеры флагов компилятора, которые влияют на короткое замыкание:

Fortran: "sce | nosce По умолчанию компилятор выполняет оценку короткого замыкания в выбранных логических выражениях с использованием правил XL Fortran. Указание sce позволяет компилятору использовать не-XL правила Fortran. Компилятор будет выполнять оценку короткого замыкания, если текущие правила позволяют это . По умолчанию используется nosce. "

Паскаль: "B - флаговая директива, которая управляет оценкой короткого замыкания. См. Порядок вычисления операндов двоичных операторов для получения дополнительной информации об оценке короткого замыкания. См. Также параметр компилятора -sc для компилятора командной строки для получения дополнительной информации ( задокументировано в руководстве пользователя). "

Delphi: «Delphi по умолчанию поддерживает оценку короткого замыкания. Его можно отключить с помощью директивы компилятора {$ BOOLEVAL OFF}».

Ewan
источник
Комментарии не для расширенного обсуждения; этот разговор был перенесен в чат .
Мировой Инженер