Как стать автором
Обновить

Комментарии 86

Примерно поэтому придерживаюсь правила 'лишних скобок не бывает'
Вы будто статью не дочитали.
Лично мне так наглядней. Оператор — явно указанная область действия. Плюс подсветка в IDE и вот это вот все.
Ошибку в примере нашел сразу.

Your milage may vary.
Простота и читаемость кода лишней не бывает. А множество скобок лишь ухудшают восприятие логики.

Вы сейчас просто разом всех лисперов идеологически оскорбили!

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

Пример 1
bool Predicate;

Predicate  = (CONST1 < a) && (CONST2 > a);

Predicate |= (CONST3 < a) && (CONST4 > a);

Predicate |= (CONST5 < a) && (CONST6 > a);

if (false != Predicate)
{
...
}; 

и
Пример 2
bool PredicateA, PredicateB, PredicateC;
bool Predicate;

PredicateA = (CONST1 < a) && (CONST2 > a);

PredicateB = (CONST3 < b) && (CONST4 > b);

PredicateC = (CONST5 < c) && (CONST6 > c);

Predicate = PredicateA || PredicateB || PredicateC;

if (false == Predicate)
{
...
};

Так же лучше не использовать строки длиннее какого-то значения, у меня не более 100 символов.
когда много строк относится к одной операции читать код тоже неприятно. Обычно лучше вынести сложную проверку в функцию, аля:
inline bool withinSomeRange(int a) {
    return (a > CONST1 && a < CONST2)
        && (a > CONST3 && a < CONST4)
        && (a > CONST5 && a < CONST6);
}

if (withinSomeRange(a)) {
// ...

Самое главное в таком подходе то, что условие именуется. С удачно выбранным именем функции во-первых её назначение понятно без чтения реализации, во-вторых проще найти ошибку в условии, нарушающем предполагаемую из имени логику
Красивый пример. Коротко, наглядно, понятно. Переписано хорошо. Можно статью джунам показывать в качестве примера.
Поэтому мне нравится подсветка скобочек в IDE.
После этого самого "!" ставишь курсор и смотришь, подсветило в конце или нет.
А можете вариант из статьи показать как будет выглядеть?
ПС: не всегда пишу в студии, так что конкретно студийное решение не всегда хорошо.
А можете вариант из статьи показать как будет выглядеть?

Студии под рукой нет, в VS Code такое же расширение у меня стоит.
image

Красных скобок в итоге много, т.е. первая и последняя в принципе красные, в конкретно этом кейсе не сильно помогает.

Здесь нужна по настоящему мощная подсветка:

В Идейке (правда только для Джавы, видимо) и подсветка скобок не нужна, варнинги будут сразу в редакторе при вводе текста. У нас это онлайн-анализатор ловит:



Любопытно, кстати, что PVS-Studio анализирует недостижимый код. Мы, например, не выдаём предупреждение, что ch <= 0x0FF3A всегда истинно, потому что оно вообще недостижимо. Видимо, когда PVS-Studio находит всегда ложное условие, оно выдаёт на него варнинг и считает, что этого условия нету и продолжает анализ дальше?

А ещё нажав Ctrl+Shift+P два раза на выражении, можно увидеть статически выведенный диапазон значений:



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


Все эти фичи есть в бесплатной версии IDEA Community :-)

PVS-Studio иногда анализирует недостижимый код, хотя многие диагностики туда не лезут. Вот именно V560 это в рамках одного выражения этого не учитывает. Так уж диагностика была исторически реализована. Хорошо это или плохо, не знаю. Всё относительно :). Менять поведение особенного смысла не вижу.

А вот, например, диагностика V522 ведёт себя иначе. Здесь будет предупреждение:
void F(int *a)
{
  if (!a && *a == 2)
    foo();
}

V522 CWE-476 Dereferencing of the null pointer 'a' might take place. consoleapplication2017.cpp 125

А здесь (в мёртвом коде) уже нет:
void F(int *a)
{
  if (a && !a && *a == 2)
    foo();
}

Здесь нет V522, зато ожидаемо есть V560 CWE-570 A part of conditional expression is always false: !a. test.cpp 125

Менять не надо, если откровенно путающих ситуаций не возникает, согласен.


a && !a && *a == 2

А если наоборот, !a && a && *a == 2

V560 CWE-570 A part of conditional expression is always false: a. test.cpp 125
НЛО прилетело и опубликовало эту надпись здесь
От этого "!", захотелось избавиться с первого взгляда на код. И только потом спросил себя, а не хотел-ли автор вынести это «НЕ» за общие скобки. Вобщем да, — лишних скобок не бывает.
Ужас, блин! Тут надо ворнинг пихать, а не другую интрепретацию делать…
Не надо такое в ворнинг.
Это базовый шаблон для PHP
if (!$x = foo()) {
    // функция не вернула not empty результат
}
// используем $x
В условиях лучше не использовать присваивания независимо от языка. Лучше писать так:
$x = foo();
if (!$x) {
    // функция не вернула not empty результат
}
// используем $x

А ещё в PHP есть операторы && и AND, || и OR, с разными приоритетами (буквенные — ниже)

!($x = 2) всегда будет false.
(!$x) = 2 — тут ошибка.
if (!$x = 2) — условие никогда не выполнится же, так как !2 даст false.

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

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

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

Речь именно что про данный случай. Ошибка то возникла как раз из-за того, что программист запутался в скобках. А было их всего две пары — для if'а и отрицания — проблемы бы вообще не было.
Ну, и никто не заставляет помнить приоритеты всех операторов, но, извините, приоритеты сложения/умножения и ИЛИ/И/НЕ должны отскакивать на зубок, на мой взгляд, и в дополнительных скобках не нуждаться.

У меня в анамнезе Паскаль, в котором приоритет операции and выше, чем операции сравнения. Я, конечно, запомнил разницу, но скобки зачастую ставлю. И не поручусь, что кто-то по запарке не прочитает неправильно.

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

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

А вот если спросить, что приоритетней — && или ||, многие растеряются.

С кем вам приходится работать? Гнать надо этих "многих", извините.


А не лучше ли написать вместо


цитата
const bool isLetterOrDigit =    (ch >= 0x0FF10 && ch <= 0x0FF19)  // 0..9
|| (ch >= 0x0FF21 && ch <= 0x0FF3A)  // A..Z
|| (ch >= 0x0FF41 && ch <= 0x0FF5A); // a..z
if (!isLetterOrDigit)


такое:


код
const bool isLetterOrDigit =    (ch >= '0' && ch <= '9')
                             || (ch >= 'A' && ch <= 'Z')  
                             || (ch >= 'a' && ch <= 'z'); 
if (!isLetterOrDigit)

?
Ещё я видел code style, где рекомендовалось в особо сложных случаях писать


код
const bool isDownCaseLetter = ...
const bool isUpperCaseLetter = ...
const bool isDigit = ...
if (!(isDigit || isDownCaseLetter || isUpperCaseLetter)) ...
Вот да. Правда, второй вариант не всегда может прокатить, не каждый символ можно так просто добавить в исходник — и в таких случаях я бы рекомендовал завести константы с говорящими именами, типа BIG_LETTER_A или FIRST_BIG_LETTER_EN, а в них уже можно любые коды засовывать, всё равно далее исходник будет читаемым.
А вот третий вариант лично я бы рекомендовал почти всем и всегда. А то и в отдельный метод IsLetterOrDigit.

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

И правда. Тогда нужно u'a', u'z' и т.п., или как там выглядят юникод литералы на С++. Писать 0xff10 — явно плохая практика.

Тогда нужно u'a', u'z'

И получится у вас ещё большая путаница. Эти символы лишь похожи на обычные, но на самом деле другие. Ссылку я не случайно привёл.
a !=a

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

Может сейчас всё иначе, но раньше использовать исходники в unicode/uft8 — хороший способ познать боль, получив на экране квадратики любимым шрифтом или проблемы с контролем версий.
Еще вариант:
код
const DWORD ranges[RANGE_COUNT][2] = {{0x0FF10, 0x0FF19}, {0x0FF21, 0x0FF3A },
{0x0FF41, 0x0FF5A }};
bool isLetterOrDigit = false;
for (size_t i = 0; i < RANGE_COUNT; ++i){
     isLetterOrDigit ||= ch >= ranges[i][0] && ch <= ranges[i][1];
}
 

Кстати, вы могли бы этот таск переклассифицировать из баги в фичреквест и не спешить закрывать. Ведь на самом деле ситуация показательная и с другой стороны: лог анализатора не дал трём с половиной программистам понять, что же происходит. Т.е. анализатор умён, спору нет, но мысль свою до людей он не донёс. Может быть, стоит на такие проблемы в логических выражениях выдавать другую ошибку? Какую-нибудь одну, красивую, которая подсветит сереньким цветом выражения, не влияющие на истинность условия, или как-то так? Ведь, насколько я понимаю, вы же всё равно строите деревья разборов этих условий, т.е. самая сложная часть уже есть и отлично работает.
Мы по возможности стараемся решать эту задачу. Иногда один общий алгоритм находит ошибку, которая затем для разных случаев разбивается на пяток сообщений для понятности. Но все случаи не предусмотреть.
Ну, тут imho невозможно выдать понятную диагностику, пока не будет понятного кода. Т.е., получается, что надо навязывать codestyle?

Я думаю, будет достаточно добавить в сообщение ", потому что ...". В данном примере,


выражение (ch >= 0x0FF21) && (ch <= 0x0FF3A) всегда false, потому что ch < 0x0FF10 и ch > 0x0FF19, что следует из выражения !((ch >= 0x0FF10) && (ch <= 0x0FF19))

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

Эмм… Это вот вы серьёзно про "многих" которые не знают у чего выше приоритет — && или ||? Они точно программисты? А бухгалтеры многие не знают что приоритетнее — умножение или сложение?

Добавил опрос.
Спасибо. Результаты… удивляют.

Джва с половиной программиста уже запуталась в казалось бы несложном выражении.

вот только знание приоритетов этих операций нужно лишь для явных нарушений правил «хорошего тона»

Утверждаете ли вы что запись 2 + 2 * 2 нарушает правила "хорошего тона"? И кто устанавливает эти правила? Чем эта запись лучше/хуже a || b && c?

как минимум тем, что не выучив приоритет арифметических операций даже школу не закончишь. Во-вторых, логических операций несколько больше, и, даже если вы способны вспомнить приоритет конъюнкции и дизъюнкции, сомневаюсь, что выражение аля x & y && z | q || w окажется для вас таким же тривиальным.

Конечно, нарушает. Найти вам примеры языков, где 2+3*2 будет равно 10?

Текст правила или статьи или чего там именно оно «конечно» нарушает в студию. Желательно со ссылкой на место откуда ваше правило взялось.
Что докажет ваш пример? Я могу выдумать язык программирования где результатом этого выражения будет «КРАСНЫЙ КРУГ,» и что? Есть языки в которых это вообще не является валидным выражением, и, опять же, что с того?

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


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


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


В исходном случае правило, что && это умножение, а || — сложение, может быть далеко не очевидно тем, кто привык к другим языкам. И подобная привычка легко может сработать как выстрел в ногу.

Я понимаю что «хороший тон», «правила приличия», «мораль» и всё такое штука субъективная и различается от социума к социуму и от индивидуума к индивидууму. Но если вы посмотрите тредик, мои прямые вопросы о том, что собеседники вкладывают в понятие «хорошего тона» применительно к &&/||, успешно игнорируются и мне видимо предлагается догадываться с помощью телепатии.

В исходном случае правило, что && это умножение, а || — сложение, может быть далеко не очевидно тем, кто привык к другим языкам. И подобная привычка легко может сработать как выстрел в ногу.


Разверните мысль, какая именно привычка может сработать как выстрел в ногу? Привыкание к языкам в которых || приоритетнее &&? Вполне допускаю что вы правы и лучше бы не привыкать к таким языкам.
НЛО прилетело и опубликовало эту надпись здесь
Расставленные скобки однозначно показывают какое выражение должно(хотел написать автор) быть записано:
(2 + 2) * 2 или 2 + (2 * 2).
Когда я учился, я знал про приоритет этих операций. Также как и чем отличается сектор диска от кластера и на каком прерывании «висит» клавиатура. Но сегодня мне нужно посмотреть документацию.
На это есть причины
  • Я отдаю отчет, что в разных языках правила могут быть разными. В статье С, который я не знаю. Вероятно, там такие же правила как в родной Java. Но хороший код работает строго определенным образом, а заказчик не поймет фразы «наверное, код правильный». Я лучше сверюсь с доками или напишу тест на это условие.
  • Я не пишу в таком стиле и переписываю, когда читаю такие конструкции. Вот уже много лет. Мне переписать проще, чем постоянно помнить порядок операндов.
    • Эта строка перегружена, ее стоит упростить. Как уже писали, выделить три boolean const — самое то. В перегруженном коде легче допустить ошибку, статья — наглядный пример.
    • Код будет изменяться, и лучше предотвратить возможность ошибиться. Через полгода кто-то захочет добавить к условию дефисы-запятые и в моих силах сделать так, чтобы он не мог написать неправильно. Даже зеленый джун, писавший на Паскале.

Да вот самая проблема в том, что Си-подобный синтаксис упёрли многие языки, но приоритет операций лежит выше синтаксиса. Поэтому хитрые выражения типа a?b?c:d:e реально вычисляются по-разному в разных языках.
Ну и про слово «перегружена» в С++ отдельная «пестня». С одной стороны, там гарантируется short-circuit исполнение операций, поэтому выражение if ( x && x->y ) гарантированно не приведёт к null pointer exception. С другой, это не так, потому что класс x может перегрузить operator&&, а чтобы его вызвать, нужно сначала вычислить оба операнда (левый и правый). Поэтому поведение выражения зависит от типа x.
Ну и вишенкой на торте — возможность перегрузки оператора «запятая». Вырванное из контекста выражение a = f(x,y) с точки зрения компилятора вообще непонятно что делает.
Попробуйте разобраться, что выдаст следующая программа:
// evil.h
class Evil {
public:
 double _val;
 Evil(double a) {_val = 1/a;}
 Evil(double a, double b) {_val = a/b;}
 Evil operator, (const Evil rhs) {return Evil(_val,rhs._val);}
 Evil operator, (double b) {return Evil(_val,b);}
};

double f(Evil a) {return a._val;}
double f(double a, double b) {return b;}

// main.cpp
#include <iostream>
#include <string>
#include "evil.h"

int main()
{
  std::cout<<f(1,2);
}
поэтому выражение if ( x && x->y ) гарантированно не приведёт к null pointer exception

К сожалению, не всегда. operator &&, будучи перегруженным, не гарантирует ленивость вычислений аргументов. Если x — некоторый умный указатель, реализующий оператор && и оператор ->, то в потрохах вполне может получиться разыменование нулевого указателя.
Несмотря на знание приоритетов, читать выражения типа (a || b && c || d) крайне трудно.
Хотя выражение (a || b && c || d) уже неприятно читать, через N коммитов оно может стать
(a || b && c || d && !e),
а позже мутировать через
(a || b && c || d && !e || (f && g))
в (a || b && c || d && !e || (f && g || !a)).
Через два года тимлид тратит четыре дня чтобы отловить плавающий баг, доходит до этой строчки, в гневе пишет git history… и видит коммиты от троих людей, двое из которых давно уволились, а init commit писал этот тимлид, когда был молодым.
И уже не у кого спросить, зачем появились скобки в хвосте выражения и нельзя по коду понять что хотели достичь последние два коммита, а контакты в телеграмме давно протухли. По старым адресам жили другие люди (тимлид на пару с битой проверял лично).
Осталось последнее средство, к которому прибегают в крайнем случае. Несмотря на всю его мощь, мало кто рисковал использовать его. Но выбора не было. Тимлид закрылся в своем кабинете, сдул пыль с бумажной стопки и открыл ТЗ…
конец бесплатного фрагмента
А почему бы не сделать выделение метода для условий и назвать его нормально?
IsDigit, IsUpperLetter, IsLowerLetter?
А почему статья на английском не на Хабре?
А Delphi/Object Pascal просто не даст скомпилироваться такому коду, пока все скобки у логических операций не проставишь.

Конкретно эту ошибку система типов Паскаля не ловит.

Доброго дня! А вы только по c? Есть ли у вас angularJS анализатор? Или может подскажет кто?
Я далек от мира фронтенда вообще и JavaScript фреймворков в частности. Поэтому вполне серьезно спрашиваю: AngularJS действительно так развился, что для него нужен особенный анализатор, отличный от plain old JavaScript анализатора?
Просветите, пожалуйста

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


В IntelliJ IDEA вижу одну инспекцию: Angular|Empty Event Handler. Что бы это ни значило.

Хороший список.
А для Pascal/Delphi не планируете сделать анализатор? Там в списке про такой язык упоминают в комментариях два универсальных анализатора. :)
Нет. Быть может дальше (но не скоро): JavaScript или PHP.
Я вот сначала подумал, что там word стал отрицательным (0x0FF00 в 16-битном представлении будет отрицательным) и поэтому «больше-меньше» местами поменялось…
А зачем люди препендят 0 к hex числам? 0x0ff21 вместо 0xff21?
Действительно странно! Тогда было бы логичнее писать 0x00ff21, чтобы не прочитать в спешке ff с лишним ноликом как 0f f…
Статья хоть и короткая, но как обычно интересная! Занятно то, что даже такой лось, как я, нашел ошибку в коде буквально с первых прочитанных символов. Так что автор кода меня слегка пугает… И, да, я бы предпочел лучшее раскрытие условий в тексте программы (например так, как автор статьи предлагает, хотя варианты в комментариях есть и получше), но при этом скобок я бы не стеснялся. Как выше написали — в гробу я видал правила приоритета в тех или иных языках. При правильном раскрытии условий скобок будет минимум, но ровно там, где нужно.
Предлагаю пообсуждать такой код, весь в скобочках. Он из проекта, о котором сейчас пишется статья. В нём есть предупреждение PVS-Studio. Какое?)

static EHTTP_HeaderParse s_ParseHeader(const char* header, ....)
{
  ....
  if (sscanf(header, "%u.%u.%u.%u%n", &i1, &i2, &i3, &i4, &n) < 4
      ||  sscanf(header + n, "%hu%x%n", &uuu->port, &tkt, &m) < 2
      || (header[m += n]  &&  !(header[m] == '$')  &&
          !isspace((unsigned char)((header + m)
                                   [header[m] == '$'])))) {
      break/*failed - unreadable connection info*/;
  }
  ....
}

Что тут обсуждать, такое переписывать надо.


А ошибка, наверно, в том, что второе header[m] == '$' всегда false.


!(header[m] == '$') && ... 
(header + m)[header[m] == '$']
Статьи содержат ошибки, иногда FA, но читатели солидарны в одном — переписать надо :D
Я бы для упрощения чтения кода написал так:
const bool isLetterOrDigit =    (ch >= 0x0FF10 && ch <= 0x0FF19)  // 0..9
|| (ch >= 0x0FF21 && ch <= 0x0FF3A)  // A..Z
|| (ch >= 0x0FF41 && ch <= 0x0FF5A); // a..z
if (!isLetterOrDigit)

Добавлять при переписывании комментарии, вводящие в заблуждение — сомнительная идея.
Т.к. (ch >= 0x0FF10 && ch <= 0x0FF19) это не 0..9, а FULLLWIDTH DIGIT 0 .. FULLLWIDTH DIGIT 9, аналогично с остальными диапазонами.

Зарегистрируйтесь на Хабре , чтобы оставить комментарий