Comments 86
bool Predicate;
Predicate = (CONST1 < a) && (CONST2 > a);
Predicate |= (CONST3 < a) && (CONST4 > a);
Predicate |= (CONST5 < a) && (CONST6 > a);
if (false != Predicate)
{
...
}; и
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)) {
// ...
Самое главное в таком подходе то, что условие именуется. С удачно выбранным именем функции во-первых её назначение понятно без чтения реализации, во-вторых проще найти ошибку в условии, нарушающем предполагаемую из имени логику
После этого самого "!" ставишь курсор и смотришь, подсветило в конце или нет.
Я вот таким чудом пользуюсь https://marketplace.visualstudio.com/items?itemName=TomasRestrepo.Viasfora там сразу без курсора видно, т.к. для каждой пары задается свой цвет.
ПС: не всегда пишу в студии, так что конкретно студийное решение не всегда хорошо.
В Идейке (правда только для Джавы, видимо) и подсветка скобок не нужна, варнинги будут сразу в редакторе при вводе текста. У нас это онлайн-анализатор ловит:
Любопытно, кстати, что PVS-Studio анализирует недостижимый код. Мы, например, не выдаём предупреждение, что ch <= 0x0FF3A всегда истинно, потому что оно вообще недостижимо. Видимо, когда PVS-Studio находит всегда ложное условие, оно выдаёт на него варнинг и считает, что этого условия нету и продолжает анализ дальше?
А ещё нажав Ctrl+Shift+P два раза на выражении, можно увидеть статически выведенный диапазон значений:
Правда он не настолько умён, чтобы по контексту догадаться выводить в хексах. Но таким образом иногда можно отдебажить варнинг и понять, что собственно произошло.
Все эти фичи есть в бесплатной версии IDEA Community :-)
А вот, например, диагностика 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
! по-другому интерпретируются приоритеты операторов: if (!$x = 2)Интерпретируется как
!($x = 2), а не как (!$x) = 2, как следует из приоритетов.
!($x = 2) всегда будет false.
(!$x) = 2 — тут ошибка.
if (!$x = 2) — условие никогда не выполнится же, так как !2 даст false.
Плохо стало уже при попытке прочитать код. Такое надо сразу переписывать, не дожидаясь диагностики анализатора (всё равно она тут "понятна" как ошибки при компиляции сложносочинённых c++ темплейтов), хотя бы в три строчки, как вы сделали при разборе.
Хотя бы банально выкинуть все лишние скобки, которые только запутывают, но не несут никакого практического смысла.
В данном случае ещё можно, но для большинства случаев я с вами не соглашусь: я в гробу видал идею запоминать приоритеты всех операторов для всех языков, а главное — не буду закладываться, что их назубок помнит тот, кому доведётся читать мой код. Лучше уж лишние скобки, а ещё лучше разбить на подвыражения, как ниже предложил Akon32.
Речь именно что про данный случай. Ошибка то возникла как раз из-за того, что программист запутался в скобках. А было их всего две пары — для if'а и отрицания — проблемы бы вообще не было.
Ну, и никто не заставляет помнить приоритеты всех операторов, но, извините, приоритеты сложения/умножения и ИЛИ/И/НЕ должны отскакивать на зубок, на мой взгляд, и в дополнительных скобках не нуждаться.
А вот если спросить, что приоритетней — && или ||, многие растеряются.
С кем вам приходится работать? Гнать надо этих "многих", извините.
А не лучше ли написать вместо
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)) ...
А вот третий вариант лично я бы рекомендовал почти всем и всегда. А то и в отдельный метод IsLetterOrDigit.
Ошибку, кстати, нашёл, нарисовав интервалы на прямой, но изначально ожидал от задачи подвох в стиле дефайна скобки в начале файла или чего-то такого.
Код '0' будет 0x30 а не как ожидалось 0xff10
unicode-table.com/ru/blocks/halfwidth-and-fullwidth-forms
И правда. Тогда нужно u'a', u'z' и т.п., или как там выглядят юникод литералы на С++. Писать 0xff10 — явно плохая практика.
Тогда нужно u'a', u'z'
И получится у вас ещё большая путаница. Эти символы лишь похожи на обычные, но на самом деле другие. Ссылку я не случайно привёл.
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];
}
Я думаю, будет достаточно добавить в сообщение ", потому что ...". В данном примере,
выражение(ch >= 0x0FF21) && (ch <= 0x0FF3A)всегдаfalse, потому чтоch < 0x0FF10иch > 0x0FF19, что следует из выражения!((ch >= 0x0FF10) && (ch <= 0x0FF19))
Подчеркнутые места сделать ссылками на соответствующие места в коде. Так как абстрактный вычислитель значений уже есть, то кажется, что это не очень сложно должно быть сделать
Эмм… Это вот вы серьёзно про "многих" которые не знают у чего выше приоритет — && или ||? Они точно программисты? А бухгалтеры многие не знают что приоритетнее — умножение или сложение?
Джва с половиной программиста уже запуталась в казалось бы несложном выражении.
Утверждаете ли вы что запись 2 + 2 * 2 нарушает правила "хорошего тона"? И кто устанавливает эти правила? Чем эта запись лучше/хуже a || b && c?
Конечно, нарушает. Найти вам примеры языков, где 2+3*2 будет равно 10?
Вы же понимаете, что "хороший тон" — это некоторая обобщённая условность. Придерживать за собой дверь — хороший тон, но если это дверь с красным кругом на броне — вход в бомбоубежище, а за вами гонится толпа зомби, то дверь можно уже и не придерживать.
Также считается хорошим тоном не засовывать взведённый пистолет себе за пояс, чтобы не выстрелить себе в… ногу.
Возвращаясь от метафор к языкам — если вы можете парой простых приёмов сделать ваш код однозначнее или понятнее для других, то использовать эти приемы — хороший тон.
В исходном случае правило, что && это умножение, а || — сложение, может быть далеко не очевидно тем, кто привык к другим языкам. И подобная привычка легко может сработать как выстрел в ногу.
В исходном случае правило, что && это умножение, а || — сложение, может быть далеко не очевидно тем, кто привык к другим языкам. И подобная привычка легко может сработать как выстрел в ногу.
Разверните мысль, какая именно привычка может сработать как выстрел в ногу? Привыкание к языкам в которых || приоритетнее &&? Вполне допускаю что вы правы и лучше бы не привыкать к таким языкам.
(2 + 2) * 2 или 2 + (2 * 2).
На это есть причины
- Я отдаю отчет, что в разных языках правила могут быть разными. В статье С, который я не знаю. Вероятно, там такие же правила как в родной Java. Но хороший код работает строго определенным образом, а заказчик не поймет фразы «наверное, код правильный». Я лучше сверюсь с доками или напишу тест на это условие.
- Я не пишу в таком стиле и переписываю, когда читаю такие конструкции. Вот уже много лет. Мне переписать проще, чем постоянно помнить порядок операндов.
- Эта строка перегружена, ее стоит упростить. Как уже писали, выделить три boolean const — самое то. В перегруженном коде легче допустить ошибку, статья — наглядный пример.
- Код будет изменяться, и лучше предотвратить возможность ошибиться. Через полгода кто-то захочет добавить к условию дефисы-запятые и в моих силах сделать так, чтобы он не мог написать неправильно. Даже зеленый джун, писавший на Паскале.
Ну и про слово «перегружена» в С++ отдельная «пестня». С одной стороны, там гарантируется 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) уже неприятно читать, через 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?
Просветите, пожалуйста
Ангуляр никогда не трогал, но мой опыт подсказывает, что даже для какой-нибудь маленькой библиотеки можно написать статические правила, которые уберегут вас от ошибочных или неоптимальных использований. А уж Ангуляр-то совсем немаленькая штука.
В IntelliJ IDEA вижу одну инспекцию: Angular|Empty Event Handler. Что бы это ни значило.
Другие анализаторы: en.wikipedia.org/wiki/List_of_tools_for_static_code_analysis
Примечание: Обновляемый список статей, в которых мы рассказываем об ошибках, найденных с помощью 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*/;
}
....
}
Я бы для упрощения чтения кода написал так:
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
Как PVS-Studio оказался внимательнее, чем три с половиной программиста