static int ParseNumber(const char* tx)
{
....
else if (strlen(tx) >= 4 && (strncmp(tx, "%eps", 4) == 0
|| strncmp(tx, "+%pi", 4) == 0 || strncmp(tx, "-%pi", 4) == 0
|| strncmp(tx, "+Inf", 4) == 0 || strncmp(tx, "-Inf", 4) == 0
|| strncmp(tx, "+Nan", 4) == 0 || strncmp(tx, "-Nan", 4) == 0
|| strncmp(tx, "%nan", 4) == 0 || strncmp(tx, "%inf", 4) == 0
))
{
return 4;
}
else if (strlen(tx) >= 3
&& (strncmp(tx, "+%e", 3) == 0
|| strncmp(tx, "-%e", 3) == 0
|| strncmp(tx, "%pi", 3) == 0 // <=
|| strncmp(tx, "Nan", 3) == 0
|| strncmp(tx, "Inf", 3) == 0
|| strncmp(tx, "%pi", 3) == 0)) // <=
{
return 3;
}
....
}Такие условия явление повсеместное, я сталкивался с ними абсолютно во всех проектах с которыми я имел дело. Вот этот пост на хабре отлично иллюстрирует сложившийся в программистском мире зоопарк того, как «можно» писать условия для if-else и каждый подход аргументирован и по своему крут, вообще пробелы рулят, табуляция отстой и все такое. Самое смешное, что он начинается словами «Условный оператор в обычной своей форме источником проблем является сравнительно редко», за подтверждением обратного отсылаю вас, опять же, к блогу PVS-studio и вашему собственному горькому опыту.
Я заметил, что в последнее время при написании if-else блоков начал повсеместно использовать подход, который формализовал только сейчас и захотел с вами поделиться. В общем и целом он совпадает с описанным в первом же комментарии к вышеупомянутому посту, но радикальней и с четкой идеей. Я использую такую технику вообще во всех условиях, не важно, сложные они или простые.
В самом общем виде правило звучит так: если в условии присутствует больше одного логического оператора, нужно задуматься о его рефакторинге. При именовании переменных выбирать имена, соответствующие бизнес-логике, а не формальным проверкам, которые очевидны из кода.
По правде, второе даже важнее. Если по поводу условия есть, что сказать, кроме того, что явно написано в нем самом, нужно подумать, можно ли это выразить названием переменной. Сравните два псевдокода:
if (model.user && model.user.id) {
doSomethingWithUserId(model.user.id);
...
}и
let userExistsAndValid = model.user && model.user.id;
if (userExistsAndValid) {
doSomethingWithUser(model.user);
...
}В первом случае у нас чисто формальная проверка значений, нам нужен user.id и мы проверяем, есть ли он, все можно оставить как есть. Во втором случае нам нужна уже валидная модель и мы объясняем это в имени переменной через термины бизнес-логики.
Такой подход имеет преимущества и при рефакторинге: если проверка понадобится дальше в методе еще раз и/или расширятся условия валидности (нужно будет, чтобы у юзера еще и емейл был обязательно) — изменения будут минимальны.
Возьмем пример тоже про пользователя, но посложнее, с которым я столкнулся буквально несколько дней назад и отрефакторил в этом стиле. Было:
if (this.profile.firstName && this.profile.lastName &&
(this.password ||
!!_.find(this.serviceAccounts, a => a.provider === 'slack' && a.accountId)) {
....
}Стало:
const hasSlack = !!_.find(this.serviceAccounts, a => a.provider === 'slack' && a.accountId);
const hasSomeLoginCredentials = this.password || hasSlack;
const hasPersonalData = this.profile.firstName && this.profile.lastName;
if (hasPersonalData && hasSomeLoginCredentials) {
....
}Конечно есть случаи, когда какие-то элементы условия надо выполнять только, если предыдущие были успешны и другие исключения, но все же: если в условии присутствует больше одного логического оператора, нужно задуматься о его рефакторинге. Большинство таких моментов можно разрешить, кстати, тернарными выражениями или локальными функциями, но насколько эти инструменты более читабельны, чем длинные if-ы, каждый решает сам.
Для меня в списке исключений стоят еще шорткаты, очевидно, если надо просто выразить что-то вроде:
if (err || !result || !result.length === 0) return callback(err);нет смысла вводить переменные.
Комментарии (126)
potan
02.07.2017 00:06+1Надо более активно использовать контейнеры с групповыми операциями, а так же переходить на языки с поддержкой pattern matching.
nikitasius
02.07.2017 00:50-25Когда сам пишешь длинные if'ы, то никогда в них не ошибаешься. Статью надо назвать:
Никогда не
пишитекопипастите длинных if-овА логика вида
a=(b>3?(c<3?b+2:2):5);и сложнее, это отдельное искуство!
belousovsw
02.07.2017 19:57+4Искусство искусством, но когда бизнес меняется и через пол года год, тебя просят подправить бизнес логику в подобном выражении a=(b>3?(c<3?b+2:2):5) волосы встают дыбом! и тихо начинаешь себя ненавидеть и проклинать за подобный когда-то написанный код. :) поэтому всегда стараюсь использовать человеко понятные имена переменных и примерно как в статье агрегировать условия в более понятные и упорядочены блоки
avost
02.07.2017 21:45+7А что в этом выражении может вызвать затруднение?
Что касается статьи… там так и не раскрыто, что делать с исходными if-ами.
if (strlen(tx) >= 4 && (strncmp(tx, "%eps", 4) == 0 || strncmp(tx, "+%pi", 4) == 0 || strncmp(tx, "-%pi", 4) == 0 || strncmp(tx, "+Inf", 4) == 0 || strncmp(tx, "-Inf", 4) == 0 || strncmp(tx, "+Nan", 4) == 0 || strncmp(tx, "-Nan", 4) == 0 || strncmp(tx, "%nan", 4) == 0 || strncmp(tx, "%inf", 4) == 0 )) { return 4; }
предлагается заменить на:
int iDontKnowWhatIsItButMustCauseReturningFour = strlen(tx) >= 4 && (strncmp(tx, "%eps", 4) == 0 || strncmp(tx, "+%pi", 4) == 0 || strncmp(tx, "-%pi", 4) == 0 || strncmp(tx, "+Inf", 4) == 0 || strncmp(tx, "-Inf", 4) == 0 || strncmp(tx, "+Nan", 4) == 0 || strncmp(tx, "-Nan", 4) == 0 || strncmp(tx, "%nan", 4) == 0 || strncmp(tx, "%inf", 4) == 0; if (iDontKnowWhatIsItButMustCauseReturningFour) return 4;
Стало понятнее, правда? :)
Я бы в таком случае вынес "страшное" условие в функцию/метод и его документировал. Заодно можно и юнит-тест к нему написать, чтобы уж совсем не оставить места для ошибки.
vintage
02.07.2017 23:21Тут стоит ввести ещё 5 временных переменных. А лучше вообще ограничиться одной регуляркой.
iCpu
03.07.2017 08:56Окей, просмотреть условие или просмотреть переменную. Без обид, это обыкновенное перекладывание шаров из урны в урну. А одна регулярка — это частный случай под конкретную задачу, и то не всегда осуществимый.
belousovsw
03.07.2017 00:15Конечно этот пример по моему сильно притянут за уши. Вот например пример из моего кода фильтра таблицы, не слишком сложный, но спустя время придется напрячься чтобы припомнить что тут и зачем, или например если сюда заглянет новый сотрудник.
if ( (typeId != 4 && typeId != 5 && showBalance != 2 && !(showBalance == 0 ? balance >= -300 : balance < -300)) || (typeId != 4 && typeId != 5 && showAct != -1 && !(showAct == act)) || (typeId != 4 && typeId != 5 && showDog != -1 && !(showDog == dog)) || (typeId != 1 && typeId != 4 && typeId != 5 && showFullAcc != 2 && !(showFullAcc == 1 ? fullAcc != 0 : fullAcc == 0)) || (showToPay != 2 && !(showToPay == toPay)) || (showFix != 100 && !(showFix == fix)) || (showComment != 2 && !(showComment == 1 || showComment == 0 ? (showComment == 1 ? cc > 0 : (cc > 0 && ccn > 0)) : (showComment == 10 ? cs > 0 : (cs > 0 && ccn > 0)))) || (!isEmpty(showManagers) && (!(showManagers.find(function (a){ return a == manager}) != undefined) == selectedMangers)) || (!isEmpty(showServices) && (!(showServices.find(function (a){ return a == services}) != undefined) == selectedServices)) || (!isEmpty(showTypeServices) && (!(showTypeServices.find(function (a){ return a == typeId}) != undefined) == selectedTypes)) || (!isEmpty(firmIds) && !(firmIds.find(function (a){ return a == fId}) != undefined)) ) { return false; }DistortNeo
03.07.2017 00:56+2Первое, что я бы сделал — это разбил один большой
ifна много маленьких, избавившись от||.
Второе — для повторяющихся условий типаtypeId != 4 && typeId != 5завёл бы по переменной.
Третье — максимально упросил условия: куда проще читатьshowAct != act, чем!(showAct == act).
И напоследок: выражение
!(showManagers.find(function (a){ return a == manager}) != undefined) == selectedMangers)мне абсолютно не понятно. РезультатомshowManagers.find(function (a){ return a == manager}) != undefined)являетсяbool, который сравнивается сselectedMangers, который, судя по имени, имеет явно не булевый тип, и остаётся только гадать, что имелось в виду в коде.
TheShock
03.07.2017 02:18+6но спустя время придется напрячься чтобы припомнить что тут и зачем
Потому что для начала необходимо выкинуть все это трюкачество с операторами и сделать явные тригеры для выхода из функции, а также вместо магических чисел ввести константы. Домен совершенно непонятный, числа словно взяты из головы, потому не все менял, показываю только идею:
оригиналif ( (typeId != 4 && typeId != 5 && showBalance != 2 && !(showBalance == 0 ? balance >= -300 : balance < -300)) || (typeId != 4 && typeId != 5 && showAct != -1 && !(showAct == act)) || (typeId != 4 && typeId != 5 && showDog != -1 && !(showDog == dog)) || (typeId != 1 && typeId != 4 && typeId != 5 && showFullAcc != 2 && !(showFullAcc == 1 ? fullAcc != 0 : fullAcc == 0)) || (showToPay != 2 && !(showToPay == toPay)) || (showFix != 100 && !(showFix == fix)) || (showComment != 2 && !(showComment == 1 || showComment == 0 ? (showComment == 1 ? cc > 0 : (cc > 0 && ccn > 0)) : (showComment == 10 ? cs > 0 : (cs > 0 && ccn > 0)))) || (!isEmpty(showManagers) && (!(showManagers.find(function (a){ return a == manager}) != undefined) == selectedMangers)) || (!isEmpty(showServices) && (!(showServices.find(function (a){ return a == services}) != undefined) == selectedServices)) || (!isEmpty(showTypeServices) && (!(showTypeServices.find(function (a){ return a == typeId}) != undefined) == selectedTypes)) || (!isEmpty(firmIds) && !(firmIds.find(function (a){ return a == fId}) != undefined)) ) { return false; }vintage
03.07.2017 07:35Немного упростил:
function has( array , needle ) { if( isEmpty( array ) ) return false return array.includes( needle ) } const TYPE_QUX = 1 const TYPE_FOO = 4 const TYPE_BAR = 5 if( ![ TYPE_FOO , TYPE_BAR ].contains( type_id ) ) { check_ballance: if( balance >= -300 ) { if( show_balance == 0 ) return false } else { if( show_balance == 0 ) break check_ballance if( show_balance == 2 ) break check_ballance return false } if( show_act != -1 && show_act != act ) return false if( show_dog != -1 && show_dog != dog ) return false check_full_acc: if( TYPE_QUX != type_id ) { if( show_full_acc == 1 ) { if( full_acc == 0 ) return false } else { if( show_full_acc == 2 ) break check_full_acc if( full_acc == 0) break check_full_acc return false } } } // ... if( has( show_managers , manager ) != selected_mangers ) return false if( has( show_services , services ) != selected_services ) return false // ...
belousovsw
03.07.2017 00:45Я не очень в C но в PHP я скорей всего сделал бы так
........ $needles[4] = ["+%pi", "-%pi", "+Inf", "-Inf", "+Nan", "-Nan", "%nan", "%inf"]; $needles[3] = ["+%e", "-%e", "%pi", "Nan", "Inf", "%pi"]; ....... foreach ($needles AS $length => $needle) { if (mb_strlen($tx) < $length) continue; $haystack = mb_substr($tx, 0, 4) if (in_array($haystack, $needle)) { return $length; } return 0; }
Думаю что в C можно что-то подобноеbelousovsw
03.07.2017 00:50не могу редактировать свои коментарии
$haystack = mb_substr($tx, 0, $length)iCpu
03.07.2017 09:18$needles[3] = ["+%e", "-%e", "%pi", «Nan», «Inf», "%pi"];
Вы с радостью заглотили ту же ошибку.belousovsw
03.07.2017 09:58Да, я как-от не вникал в набор данных, просто скопипастил :(
Просто хотел избавится от лишних блоков else ifiCpu
03.07.2017 10:26+1Есть очень хорошая игра для компании. Keep talking and nobody explodes. Скачайте мануал и посмотрите, сможете ли вы избавиться от вереницы лишних if-else.
Статья написана о частных случаях. В частном случае можно было вообще проверять посимвольно. Но, в общем, не ясно, что лучше: вереница if'ов или вереница переменных с промежуточными результатами. Или именованные методы с проверкой.
Моё мнение, что каждый отдельный случай требует своего подхода. Где-то лучше впихнуть всё в один if
if (object && object->isValid() && object->hasAnimation() && object->isNotAnimatedNow()) { object->animate(); }
а где-то стоит разнести по переменным
string filename1 = FileSystemPath(file1->path()).makeAbsolute().string(); string filename2 = FileSystemPath(settings.load(KEY).toString()).makeAbsolute().string(); if (compare(filename1, filename2, FileSystemPath::CaseSensitivity())) { DoSomething(); }
avost
03.07.2017 00:56Можно-то можно, но ведь оно так не стало понятнее. Зато, как уже сказали, стало менее оптимизированно. Лучше уж в регэксп, действительно, переделать. Скомпилированный.
Scf
03.07.2017 12:51В данном случае лучше вынести все строковые константы на сравнение в отдельный массив и итерироваться по нему.
Но универсальных решений все равно нет — вышеприведенный пример с большим if-ом, к примеру, имеет максимальное быстродействие, возвращая 4 сразу, как только найдет нужную строчку.avost
03.07.2017 13:29Третий раз предлагают массив с иттерациями как лучшее решение. Очень странно. Оно хуже, чем if-ы по всем параметрам. Не более понятно и хуже по быстродействию. А если уж так сильно хочется использовать какой-нибудь контейнер, то гораздо лучше запихать их в хешмапу.
Scf
03.07.2017 13:37По быстродействию хуже, но совсем ненамного — на один цикл и немного арифметики указателей.
Стандартная хешмапа для такого маленького набора строк ни разу не факт, что быстрее — вычисление хеша дороже, чем сравнение строк. Еще memory locality — хешмапа в плюсах построена на односвязных списках или с открытой адресацией?avost
03.07.2017 14:42Так хеш же единственный раз для образца будет вычисляться. Это дешевле, чем куча сравнений строк. А хэшмапу и свою можно для такого случая написать. На массиве и с гарантированным отсутствием коллизий.
Barafu
02.07.2017 01:53-15Есть проблемы.
let userExistsAndValid = model.user && model.user.id; if (userExistsAndValid) { doSomethingWithUser(model.user);
В многопоточном коде не работает. Был валидный, к следующей строке стал не очень. В сетевом — тоже. При неаккуратном рефакторинге между первой и второй строчкой может вклиниться удаление юзера. Использование переменной userExistsAndValid как-бы побуждает использовать её значение в дальнейшем. В то время как её нужно каждый раз перевычислять. Уж если так делать, то нужно её после блока if сразу сбрасывать в некоторое значение "ни да ни нет".
saboteur_kiev
02.07.2017 02:58+4Статья же не об этом.
Как бы очевидно, что если есть многопоточный код, то в нем есть и транзакции/мутексы/как_оно_называется_в_вашем_языке, в которых и пишешь то, куда никто не должен вклиниваться.
maxpsyhos
02.07.2017 03:54+30Простите, а разве сложное-навороченное выражение выполняется как атомарная операция? Ведь между операциями чтения model.user и model.user.id тоже может вклиниться удаление юзера.
dimack
03.07.2017 11:33-1В данном случае model.user это структура с полем id, которая была атомарно получена из хранилища. Не вижу тут никаких проблем. Видимо, подразумевается, что хранилище возвращает либо модель либо null, false… Поэтому сначала проверка на то, является ли результат собственно структурой.
mayorovp
03.07.2017 16:22Независимо от того, является ли model.user разделяемой или нет, потоко(не)безопасность от наличия лишней переменной не зависит. Для разделяемого поля оба варианта будут содержать гонку, для неразделяемого (ваш случай) оба варианта будут безопасными.
dimack
03.07.2017 23:15Ой, я думал это коммент к статье, не сразу понял при чем тут потоки:) Так-то да.
AllexIn
02.07.2017 17:11+2{ bool userExistsAndValid = model.user && model.user.id; if (userExistsAndValid) { doSomethingWithUser(model.user); }
Ну а про потоки вы вообще бред несете
Fatik
02.07.2017 19:58+5А что помешает пользователю перестать быть вадидным после проверки в первой строчке и перед вызовом doSomethingWithUserId во второй строчке, в этом коде?
if (model.user && model.user.id) { doSomethingWithUserId(model.user.id); }
SerafimArts
02.07.2017 05:37+7Статья, конечно, полезная, но есть ощущение, что просто пересказана одна из страничек книги господина Макконнелла.
Так что всем тем, кому понравился данный топик — настоятельно рекомендую к прочтению эту чудотворную книгу. Там не только про подобные условия есть, а вообще много чего на тему читаемого кода.
AllexIn
02.07.2017 17:12+1Я её уже 11 лет не могу полностью осилить…
Может время пришло… Попробую еще разок.
osharper
02.07.2017 20:36+1я бегло прошелся по Макконнеллу перед написанием статьи (блин, три сдвоенных согласных это круто) он, безусловно, пишет про самодокументирующийся код, смысловые названия переменных и все такое, но на условиях он не делает какого-то отдельного акцента на условиях.
Проблема в том, что большинство тех, кто более или менее следуют макконнелловским принципам, на условиях сразу сползают на уровень кода в районе уроков по информатике в девятом классе. Но это субъективный опыт конечно.
minamoto
03.07.2017 16:29Я, может быть, не до конца понимаю разницу, но очень похожие рекомендации я встречал в его книге как минимум несколько раз (сейчас как раз понемногу читаю).
Как пример, подраздел «Последовательности операторов if-then-else» раздела «15.1. Операторы if». Как раз про то, что нужно сворачивать множественные условия в единичные функции проверки для удобства чтения и понимания.
Еще пример — подраздел «Упрощение сложных выражений» раздела «19.1. Логические выражения» — о выведении дополнительных переменных, сокращающих количество условий методом группировки по логическому соответствию.
Как по мне — очень похоже на содержимое статьи.
andreycha
03.07.2017 15:38+1. И «Рефакторинг» Фаулера в придачу. В частности: https://refactoring.com/catalog/decomposeConditional.html
dipsy
02.07.2017 08:07+3Первый пример просто чудовищный, я бы с таким человеком в одном коворкинге кодить не сел. Просто когда рука поднимается такое писать, ну это уже всё, приехали. Вот хотя бы так, что ли:
static int ParseNumber(const char* tx) { static const char* Is4[]= {"%eps", "+%pi", "-%pi", "+Inf", "-Inf", "+Nan", "-Nan", "%nan", "%inf", NULL}; static const char* Is3[]= {"+%e", "-%e", "-%pi", "%pi", "Nan", "Inf", "%pi", NULL}; .... else if (contains(Is4, tx)) { return 4; } else if (contains(Is3, tx)) { return 3; } .... }firk
02.07.2017 21:14+1В исходном коде явно видна попытка оптимизировать по скорости (проверка strlen перед перебором), ваш же вариант прямо противоположен (включая и то что функция contains будет медленнее чем захардкоденые проверки).
TheShock
02.07.2017 21:44-1Ту странную проверку на длину можно и сюда добавить, а вот «функция contains будет медленнее чем захардкоденые проверки» — вы уверены, что будет какое-либо значительное увеличение скорости?
firk
02.07.2017 23:34+1Разница конечно маленькая (как от strlen так и от реализации сравнений), но всё же именно она могла быть причиной того кода. Хотя может это и просто первый пришедший на ум вариант кого-то, кто об этом вообще не думал.
bogolt
02.07.2017 21:51Тогда можно запихнуть проверки в макросы. Будет более читаемо и сохранится возможность оптимизаций компилятором.
dipsy
03.07.2017 10:38ничего не мешает добавить минимальный ожидаемый strlen аргументом в contains или перед ним такую же проверку сделать, суть не в этом же.
if (strlen(tx) > 3 && contains(...))
osharper
02.07.2017 21:48если производительность не критична, то да, я тоже считаю, что как-то так и надо делать, все понятно. В противном случае можно оставить все как есть, но свести в итоге все к
else if(isThreeCharsSpecialSymbol(tx)) { return 3; } else if (isFourCharsSpecialSymbol(tx)) { return 4; }
Или даже написать
isNCharsSpecialSymbol(tx, n)и переделать все в цикл :)
iCpu
03.07.2017 09:39Что разницы, что вы всё переделали по-своему, если вы затянули ошибку из того кода?
static const char* Is3[]= {"+%e", "-%e", "-%pi", "%pi", «Nan», «Inf», "%pi", NULL};
Что толку бить себя в грудь и рвать тельняшку, если вы тут же ловким движением обсираетесь? Ладно, автор оригинального кода протупил при копипасте, вам же специально подсветили «вот они, ошибки, не делайте их», и вы с размаху их делаете.
При этом проверка одинаковых условий анализатором куда проще, чем проверка содержимого массивов. При этом, компилятор может выплюнуть лишнюю проверку. При этом, будь у вас не 6 блоков строчек, а 600, что ваш, что тот код были бы одинаково плохо читаемы, потому что много буков. Но об этом мы не думаем, у нас на конкретной задаче мир закончился, и океан упирается в край земли.dipsy
03.07.2017 10:55Это был псевдокод, если вы не поняли, и суть была не в устранении ошибки. Я вам ещё намекну на будущее, функция contains нигде не определена, пример даже не скомпилится, т.е. я получается 2 раза обосрался, пользуясь вашей терминологией.
Но тут есть нюанс, кто-то постоянно кладет себе в штаны, это даже может быть незаметно со стороны, пока не потечет (правда code smell при ревью кода, конечно сшибает с ног), а кто-то понимает, что так делать не надо. А с виду оба одеты, в штанах, оба программисты.iCpu
03.07.2017 11:12Не соскакивайте с темы. Ваш пример ни капельки не спасает от человеческого фактора. Вы, писавший, что код нужно переписать иначе, чтобы было проще обработать условие, тут же оставляете ошибку старого кода.
И какая мне, тимлиду, разница, как вы сделали проверку: через массив или перебором в строчку, если ошибка как была, так и осталась?
Я не оправдываю тот код, в нём достаточно феерии, но чем вы лучше? Разве что рассуждениями о штанах.dipsy
03.07.2017 12:55А я вам больше скажу, ничего не спасет от человеческого фактора. Вы сейчас настолько мощно путаете теплое с мягким, что я даже не знаю как реагировать, похоже на жирный троллинг. Ну это как, я не знаю, сравнивать двух водителей, один раздолбай, гоняет, не пристегивается, второй аккуратный, вежливый. В итоге обоих в какой-то момент разматывает об встречный грузовик (ну бывает же, даже с самыми аккуратными, при стечении определенных обстоятельств, согласитесь). И тут выходите вы, весь в белом, и с укоризной вещаете, дескать что толку что он такой весь аккуратный был, вон теперь только кишки по асфальту.
adic3x
02.07.2017 10:07+7Я конечно не программист, но спрошу: почему не вынести проверку в отдельную функцию с понятным названием? А в случае с
user— в отдельный метод?
dimkss
02.07.2017 11:01+1Если просто пытаться объяснить, то все сложно )
Вся статья (имхо) сводится к: пишите код проще, проще для понимания.
Плюс подхода когда проверка не в функции а в коде, в том что весь код у тебя сразу перед глазами, не надо тратить время на лазанье по функциям, проверкам что именно эта функция делает, и т.д.
Минус — ну это очевидно — код становиться большим, но как-бы читаемым, и снова (блин) сложным.
Dionis_mgn
02.07.2017 20:40+2Если условие необходимо проверять во многих местах, так и делают. Вместо переменной
let userExistsAndValid = model.user && model.user.id;вводят функцию
function userExistsAndValid { return model.user && model.user.id; }
TheOleg
08.07.2017 13:34Скорее всего, тогда будет или куча новых мелкых методов, которые опять же придётся сравнивать, либо
function x(){
if(x == 1) {
...
}
}
станет
function x(){
if(y()) {
...
}
}
function y(){
return ... && ... && ... && ... && ... && ... && ... && ... && ... && ..;
}
Что не то чтобы проще
vintage
02.07.2017 10:12+6В первом случае у нас чисто формальная проверка значений, нам нужен user.id и мы проверяем, есть ли он, все можно оставить как есть. Во втором случае нам нужна уже валидная модель и мы объясняем это в имени переменной через термины бизнес-логики.
В том-то и дело, что по логике нам тут нужна не "валидная модель", а "модель с идентификатором". Валидность может включать в себя проверку кучу условий по разным полям, которые тут не нужны. Вы могли бы назвать эту переменную "hasUserWithId", но это ничем не лучше "model.user?.id". Отсюда вывод: вводить переменную с говорящим названием нужно лишь тогда, когда у вас не получается написать говорящий код :-)
sand14
02.07.2017 10:34+2Отсюда вывод: вводить переменную с говорящим названием нужно лишь тогда, когда у вас не получается написать говорящий код :-)
Верно. И еще очень часто приходится наблюдать, что когда вводят переменные с "говорящим" названием, то для нее не могут подобрать действительно говорящее и точное название.
Что говорит о том, что введение таких переменных это попытка лечить симптомы в коде, а не его архитектуры.
Если у модели или сущности нет метода IsValid(), то вынесение простыней условий в якобы "говорящие" переменные только засоряет код и ведет в side-эффектам из-за того, что часть условий вычисляется, когда это не нужно.
osharper
02.07.2017 21:12в данном случае да, ничем не лучше, но не все пользуют C# или что-то еще где есть оператор
?.Валидность, о которой вы пишете, должна проверяться внутри модели, валидность в данном контексте выполнения, как мне кажется, допустимо оформлять так.
sand14
02.07.2017 10:29+13Предложенный подход тоже не лучший.
Когда плодятся локальные переменные вида "let userExistsAndValid = model.user && model.user.id",
это признак того, что userExistsAndValid должен быть вынесен в отдельный метод, функцию, extension, и т.д.
Они засоряют код, ведут к копипасте (в местах, где нужно выполнить такую же проверку; а что вы будете делать, когда в 10 местах по всему проекту проверяются на null user и user.id, и вдруг для проверки нужно проверить третий параметр?).
Должно быть что-то вроде такого: user.IsValid()
Например, в C# это можно сделать с помощью extension, который заодно проверил user и на null тоже.
Способов полно во всех языках для этого.
"Конечно есть случаи, когда какие-то элементы условия надо выполнять только, если предыдущие были успешны и другие исключения"
А эту проблему нужно решать: если какое-то условие проверяется, когда оно не нужно, то это не только в целом "неправильно": вычисление в таком случае может проводиться на неконсистентных для контекста данных, что может привести к side-эффектам, крешам, и т.д.
Решать можно, вставляя в проверку условия лямбду с кодом, который вычисляет значение.
При необходимости можно использовать Lazy.osharper
02.07.2017 20:29спасибо, все это верно. Я просто старался примеры подобрать с использованием самых простых инструментов. Сам активно использую и C#, и JS, поэтому активно пользуюсь всеми упомянутыми вами инструментами. И
user.IsValid()имеет такую же ценность какuserExistsAndValidв плане code quality, особенно если у вас одно и то же понятиеIsValidв десяти разных местах кода. А вот если это выливается в повсеместныеif (user.IsValid() && user.lastName..., то, возможно, каждый раз стоит иметь перед глазами всю цепочку логических операций.
Я тут не пришел к вам со светочем абсолютной истины, просто вот придумал делать так, понравилось, выразил в словах и написал на Хабр :) Не думаю, что может быть какой-то однозначно "лучший" подход вообще
sand14
04.07.2017 13:16Я хотел донести мысль, что само по себе использование промежуточных локальных переменных не решает проблему, а в чем-то даже усугубляет (лишние вычисления, в т.ч. с возможными side-эффектами), и вообще напоминает стиль "кодинга" эдак из 80-90-х.
В то же время с длинными условиями надо что-то делать.
Как вариант, предлагаю в качестве промежуточных переменных использовать лямбды (или, если говорить о C# 7- локальные функции) — т.е., в месте объявление переменной не вычислять ее значение, а задать правила вычисления.
Если какая-то переменная встречается в длинном условии более одного раза, то лямбду обернуть в Lazy.
При соблюдении пары этих правил сохраняется преимущество предложенного вами подхода, и устраняются возможные side-эффекты.
А если совсем по-хорошему, то, как правило, появление длинных условий свидетельствует, скорее, о то, что на этапе проектирования было сделано что-то не так.
Dionis_mgn
02.07.2017 22:11Вы говорите о несколько другом случае. Длинные условия — это одно. Повторяющиеся условия — это другое. Длинные условия (метрика автора мне вполне нравится — более 2 операторов) надо выносить в переменную. Иначе интерфейс рискует быть засраным по самое не могу почем зря. Да и локальность кода сохранится. Повторяющиеся условия — выносить в методы/функции (и тут уже не важно, насколько это условие длинное).
sand14
04.07.2017 13:08Вы правы насчет того, что не нужно выносить в интерфейс (контракт) то, что не имеет смысла на уровне контракта, а имеет смысл только в контексте какого то метода.
ainoneko
02.07.2017 10:40+4А как быстро выполняется
?!!_.find(this.serviceAccounts, a => a.provider === 'slack' && a.accountId)
Если hasPersonalData ложно (нет имени или фамилии) и пароля тоже нет, то в исходном варианте на этом всё заканчивается, а в новом — всё равно вызывается !!_.find.osharper
02.07.2017 20:15-1ну это же просто underscore helper, его можно было бы легко заменить на [].some(), просто я оставил все как было, кроме того, что непосредственно к статье не относится. Ну а массив this.serviceAccounts, как может быть можно догадаться из контекста, редко бывает больше 10 элементов.
alix_ginger
03.07.2017 14:21+1Это частный случай. В общем случае, вычисление одного из условий может быть ресурсоемким. По-моему, лучше заменять сложные условия не на переменные, а на функции/методы, чтобы не производить лишних вычислений
kekekeks
02.07.2017 12:05+14В подходе с "запишите всё в переменные перед условием" есть проблема, если у вас цепочка из
else if. Вынос всех этих вычислений до цепочки, во-первых, не всегда возможен, во-вторых, может дать проблемы с производительностью из-за вычисления кучи всего разного, что не потребуется при срабатывании первого условия. И вот второе хорошо видно на вашем же примере из начала статьи.
И эти люди нам пытаются продать анализатор кода.
dimkss
02.07.2017 14:02+2Поспорю немного с вами.
Вынос всех этих вычислений до цепочки, во-первых, не всегда возможен
Так автор же об этом и пишет, нет?:
Конечно есть случаи, когда какие-то элементы условия надо выполнять только, если предыдущие были успешны и другие исключения…
… во-вторых, может дать проблемы с производительностью из-за вычисления кучи всего разного
Читаемость кода и производительность (мне кажется) в большинстве случаев находятся по разные стороны.kekekeks
02.07.2017 19:19+2Читаемость кода и производительность (мне кажется) в большинстве случаев находятся по разные стороны.
Но данный совет может привести к деградации производительности на несколько порядков (особенно если где-то в конце цепочки else if идёт запрос к базе, а в предыдущих его нет) при весьма сомнительном улучшении читаемости в стиле паскалевского "объявить все переменные в начале процедуры, так всем понятнее будет".
dimkss
02.07.2017 19:25Ага, с этим не поспоришь. Если все делать идеально читаемым, программа станет мее-е-едленным динозавром. С минутными, а то и более задержками.
Bonart
04.07.2017 15:00+1Это неправда и любимая отмазка write-only писателей.
dimkss
04.07.2017 15:10Вообще-то это гипербола. Или вы имеете ввиду быстроту написания кода, а не его выполнение?
Пример:
Самые быстрые куски кода пишутся на asm иногда, или похоже на asm, без объектов, вызовов функций, лямбд и т.д.
Что вы думаете?
LynXzp
05.07.2017 09:50Самые быстрые куски кода пишутся на asm иногда, или похоже на asm, без объектов, вызовов функций, лямбд и т.д.
static inline спасут от боязни выделять код в функции. И еще внезапно иногда лучше noinline в т.ч. и по производительности.
И так говорите будто на более низкоуровневых языках (без лямбд и объектов) нельзя писать читаемый код.
Bonart
05.07.2017 11:34+1Вообще-то это гипербола. Или вы имеете ввиду быстроту написания кода, а не его выполнение?
Очень просто: идеальная читаемость НЕ делает программу медленной.
Ваш тезис не преувеличение (гипербола), а прямая дезинформация
Самые быстрые куски кода пишутся на asm иногда, или похоже на asm, без объектов, вызовов функций, лямбд и т.д.
Это иллюстрирует совершенно другое явление: код с низкоуровневыми ручными оптимизациями читается плохо.
И нужен такой код далеко не всегда даже на крайне бедных платформах.
В любом случае оптимизировать корректный код проще, чем корректировать оптимизированный.dimkss
05.07.2017 11:49Вам не кажется что тут взаимоисключающие утверждения?
Идеальная читаемость НЕ делает программу медленной.
Код с низкоуровневыми ручными оптимизациями читается плохо.
dimkss
05.07.2017 11:58-1Комментарии нельзя редактировать, так-что:
Это иллюстрирует совершенно другое явление: код с низкоуровневыми ручными оптимизациями читается плохо.
Я понял в чем мы не сходимся. Для меня это все, это один (единый) код. С которым надо работать, который подлежит и оптимизации, и редактированию для хорошей читаемости.Bonart
05.07.2017 13:42Для меня это все, это один (единый) код. С которым надо работать, который подлежит и оптимизации, и редактированию для хорошей читаемости
Нет. Читаемость нужна априори и везде.
Низкоуровневая ручная оптимизация нужна там и только там, где имеющиеся программные инструменты не позволяют добиться требуемой производительности.
"Преждевременная оптимизация — корень всех зол в программировании" (с)
TheShock
02.07.2017 19:39+1особенно если где-то в конце цепочки else if идёт запрос к базе, а в предыдущих его нет
Запрос внутри хрен знает какого условия else-if? Даже страшно подумать, что такой говнокодище может достаться для поддержкиmayorovp
03.07.2017 16:30Не вижу говнокода в проверке вида
if (someValue || (someValue = requestFromDatabase("SomeValue")))если она встречается в коде в единственном числе.
Bonart
05.07.2017 11:36Читаемость кода и производительность (мне кажется) в большинстве случаев находятся по разные стороны.
Неправильно вам кажется.
В вашем случае к потере производительности приводит использование переменных вместо функций, что никакого отношения к повышению читаемости не имеет.
splav_asv
02.07.2017 20:48+1тогда можно начиная с C++17 If с инициализацией использовать и определять выражения в ней.
osharper
02.07.2017 20:54да, это другие люди. Для меня блог pvs-studio является естественным источником кода с багами из-за дефицита code quality для статьи на Хабре. Если подскажете лучше — будет интересно взглянуть. Хотя если за эту статью мне предложат денег (бывает такое, что предлагают денег за статью постфактум?), не откажусь, честное слово :)
Вообще я активно использую лямбда выражения, но не стал писать кода с их использованием, ибо не во всех языках они есть. Любые более-менее сложные проверки более читаемые в теле функций, а если проверка еще и встречается в нескольких местах — прямая дорога в метод.
Вообще, если фанатично следовать этому правилу (хотя я сам так не делаю конечно), всегда можно переписать
else ifкак
else { let firstConditionIsTrue = longCalculatedBoolean(); let hasSecondCondition = secondCondition != null; if (firstConditionIsTrue && hasSecondCondition ) { .... } }
возможно это и правда будет читабельней в каких-то случаяхosharper
02.07.2017 21:17блин, не увидел, что с кодом случилось
else { let firstConditionIsTrue = longCalculatedBoolean(); let hasSecondCondition = secondCondition != null; if (firstConditionIsTrue && hasSecondCondition ) { . ... } }
reforms
02.07.2017 13:47+1Спасибо за статью. Есть пару вопросов и замечаний:
1) последний пример последнее условие !result.length === 0 не очень читаем
2) на сколько плохо сделать метод в моделе model.hasUserWithId? И сразу связанный с ним — id = 0 не допускается?
3) пример хоть и не Ваш, но раз приводите — почему if else везде вместо ПРОСТО if если каждый блок с прерыванием return?
4) что совсем не понятно — это пример с firstName/lastName и accounts — почему не выделить логику в отдельный метод и хорошенько задокументировать, мол, (1) должны быть указаны имя/фамилия, (2) введён пароль и (3) что ещё
tema_sun
02.07.2017 15:59+13) Без else, все if'ы будут вычисляться и проверяться каждый раз, а так только до первого совпадения.
Я тоже за соотвествующие методы, вместо таких локальных переменных. Они, конечно, симпатичнее выглядят, чем просто сравнение параметров, но имхо тоже костыли.TheShock
02.07.2017 19:243) Без else, все if'ы будут вычисляться и проверяться каждый раз, а так только до первого совпадения.
Не будут, метод закончится на первом return, все остальные if не сработают.
Idot
02.07.2017 16:02А если просто Case или Switch написать?
yarric
02.07.2017 18:43+2А можете написать, как можно первый же пример из статьи преобразовать в адекватный switch?
TheShock
02.07.2017 21:42Не знаю, на сколько такое работает в оригинальном языке из статьи, но во многих динамических — довольно просто:
switch (tx) { case "+%pi": case "-%pi": case "+Inf": case "-Inf": case "%inf": case "+Nan": case "-Nan": case "%nan": return 4; case "+%e": case "-%e": case "%pi": case "Inf": case "Nan": return 3; }
С другой стороны, судя по тому отрезку — это просто такой очень кривой способ реализовать strlen для определенных значений.
akzhan
02.07.2017 19:31+1static int ParseNumber(const char* tx) { static const char** choices = [ "%eps", "-%pi" ]; static const char** noises = [ "%Nan", "Inf" ]; .... if (contains(choices, tx) ) { { return CHOICES; } if (contains(noises, tx) ) { return NOISES; } .... }
Egor92
02.07.2017 19:58+1Я во время код-ревью также прошу выносить составные выражения из блока if в переменную и давать ей чёткое и ясное имя. Всегда говорю своим коллегам, что код должен читаться, как книжка.
Dmitri-D
02.07.2017 19:58+2Этот код — прсто треш. Проверку на токены нужно делать токенизаторами, а вовсе не strncmp. Это к тому же еще и работает быстре, а заодно и ошибок будет меньше — потому что все токены (последовательности) будут в одну колонку по вертикали — легко ее отсортировать по порядку и увидеть косяки.
Пример неплохого токенизатора — re2c :)
Почему токенизатор работает быстрее — посмотрите, в вашей исходный код — там мало лидирующих символов — это плюс минус, процент, I и N, т.е. всего пять. Вы можете начать проверку первого символа во входном потоке с этих пяти символов и ветвиться дальше, если есть совпадение. Если писать такое ручками, то ветвление будет выглядеть довольно замысловато, и его будет сложно обслуживать, например сложно будет добавить новую последовательность. К счаcтью есть токенизатор, который сделает это всё за вас.
imikh
02.07.2017 19:58+1Написано всё верно, но вместо переменных нужно использовать функции или методы.
Andreyika
02.07.2017 19:58+1Краткий анонс статьи:
Нашел тут исходник какого-то математического софта на плюсах, ниче не понятно… сейчас я вам покажу, как правильно и читаемо надо было написать этот код на примере подсчета суммы двух введенных чисел на js.
Почему-то автор не взял пример из начала статьи и не отрефракторил его с элементами бизнес-логики.
Можно, конечно, вынести содержимое условия в функцию
elseif ( strlen(tx) >= 3 && checkStrLen3(tx)) {}, убрав содержимое в функцию "как есть", но тогда будет хороший if, плохой return.
Можно пойти дальше, заменив strncmp(tx, "+%e", 3) == 0 на функцию isE( tx ), но условие все равно будет из кучи функций
return isE(tx) || isPi(tx) || isNan(tx) || isInf(tx) || isPi(tx)
Стало ли проще найти дубликат? не факт.
const hasSlack = !!_.find(this.serviceAccounts, a => a.provider === 'slack' && a.accountId); const hasSomeLoginCredentials = this.password || hasSlack;
Норм. Пароля может не быть у десятка человек, а поиск колбэком по serviceAccounts — каждому. Зато читаемо.
По итогу — в названии статьи про if, по тексту — похвальная попытка закопать кишки бизнес-логики поглубже, по факту — попытка не очень удачная — все model.user.id мясом наружу валяются в переменных строками выше.
osharper
02.07.2017 21:39у меня в изначальном варианте статьи был вариант рефакторинга из "ниче не понятно". но я не стал публиковать его по трем причинам:
- Вы правы и опыта в плюсах у меня немного, последний раз писал активно на них лет восемь назад, а вникать заново в контекст не хотелось
- У меня нет понимания big picture продукта и даже контекста этого метода
- Я скорее не согласен с таким подходом к парсингу принципиально, и соглашусь с Dmitri-D, что в общем случае нужно использовать токены, но см п. 1 и особенно 2
Коллбэк в JS коде в данном случае совсем не страшен, это синхронная функция, фильтрующая уже имеющийся массив с небольшим числом элементов.
В данном случае, как мне кажется, призыв к инкапсуляции не обоснован, так как мы в контексте функции собираемся использовать public-данные другого объекта, и нам решать, валиден он в нашем контексте или нет. Вот если такая проверка не отвечает принципу DRY, то это уже другой разговор.
osharper
02.07.2017 21:50вот в комментарии примерно попытался обозначить свой подход к первому листингу из статьи
Laryx
02.07.2017 19:58+1Полностью согласен с главной мыслью.
Оптимально — в операторе If должно быть только одно условие. Максимум — два. Если их там больше — целесообразно все их вынести в функцию с осмысленным названием.
VMichael
02.07.2017 20:22+1Никогда не говори никогда, как говориться.
Разные бывают ситуации и разные подходы.
Бывает, что и длинный IF вполне себе понятен и не требует рефакторинга и выноса в переменные или функции.
Зачем использовать слова «никогда».
С них обычно и начинаются засады.
Жизнь гибче и многогранней.osharper
02.07.2017 20:58ну это просто мое понимание того, как должно выглядеть название поста, в который хочется зайти и написать комментарий, так-то я пишу, что все не так однозначно конечно :)
Apx
02.07.2017 20:44+2Последний пример "когда можно не выносить" убил весь дух статьи… Играем, не играем, рыбу заворачиваем. Надо писать в одном стиле, особенно если в команде уже есть code conventions.
osharper
02.07.2017 22:03это все сведется с бесконечным
const isError = ....; if(isError) { .... }. Никакой дополнительной информации вы из этого не получите, условия просты как две копейки — зачем такой формализм?Apx
02.07.2017 22:09+1Ну и писать константу для обычной проверки на null или undefined я тоже считаю избыточным. Не то что я прямо предлагаю экономить память на указатели, но меня учили по старому: есть данные — юзай.
ivkomn
03.07.2017 10:49-3Не так страшно длинное условие в этом самом conditional statement'e
как точка выхода из функции посреди функции.
«Так верстают только ...»Dionis_mgn
03.07.2017 11:48+1Вы из тех, кто считает, что точка выхода должна быть всего одна?
ivkomn
03.07.2017 13:01Вы-таки предпочитаете делить людей на группы «кто за» и «кто против»?
Dionis_mgn
03.07.2017 13:23Ваша категоричность бескомпромиссна. Потому и спросил.
А люди и без моей помощи себя отлично делят на группы.ivkomn
03.07.2017 13:33Мнение каждого человека категорично и безкомпромисно в первом приближении,
если это действительно его мнение,
иначе — это заискивание перед (собеседником|окружающими).Idot
03.07.2017 14:03А варианты "не знаю" и "не уверен"- Вы в принципе не рассматриваете?
Бескомпромиссность мнения по вопросу, которого человек не знает — признак самодурства.
kostus1974
03.07.2017 13:51strncmp(tx, "+%pi", 4) == 0 || strncmp(tx, "-%pi", 4) == 0
надо реализовать до логического конца: объявить массив подстрок и определить функцию, проверяющую вхождения в строку. типа IsContains(tx, длина tx, указатель на массив подстрок, длина массива подстрок).
тогда всё будет проще.
Gumanoid
03.07.2017 19:39+1С простыми примерами всё понятно, а как лучше переписать вот это условие в GCC?
TheShock
03.07.2017 21:27Вынести в отдельную процедуру, которая возвращает true или false. И потом разбить на множество ретурнов. Маленькие независимые блоки читаются значительно легче одного огромного. Ну и участки, которые вычисляются несколько раз — записать в переменную и использовать уже ее.
На самом деле если присмотреться, то это не один огромный атомарный блок, там есть множество независимых условий. Вот допустим блок не должен выполняться, если «in == 0». Почему бы его и не выделить в отдельную логическую группу?
Код получился на больше строк, но из плюсов:
— Явно видно в какой момент остановится выполнение
— Значительно легче добавить условие в середину
— Не так легко запутаться в скобочках
— Значительно легче прокомментировать появления каждого условия.
— Можно рефакторить блоками, вынося их в отдельную процедуру
bool core_condition() { // мы должны тут выйти, потому-что ... if (in == 0) { return false; } // а тута... if (GET_CODE (in) != SUBREG) [ return false; } // If subreg_lowpart_p is false, we cannot reload just the // inside since we might end up with the wrong register class // But if it is inside a STRICT_LOW_PART, we have // no choice, so we hope we do get the right register class there if (subreg_lowpart_p (in) == false && strict_low == false) { return false; } var subreg = SUBREG_REG(in); var subreg_mode = GET_MODE(subreg); // bla-bla #ifdef CANNOT_CHANGE_MODE_CLASS if (CANNOT_CHANGE_MODE_CLASS(subreg_mode, inmode, rclass)) { return false; } #endif // bla-bla if (!contains_allocatable_reg_of_mode[rclass][subreg_mode]) { return false; } // bla-bla if (CONSTANT_P(subreg) || GET_CODE(subreg) == PLUS) { return true; } // bla-bla if (strict_low) { return true; } // ....... // ....... // ....... #ifdef CANNOT_CHANGE_MODE_CLASS if (!REG_P(subreg) || REGNO(subreg) >= FIRST_PSEUDO_REGISTER) { return false; } if (!REG_CANNOT_CHANGE_MODE_P(REGNO(subreg), subreg_mode, inmode)) { return false; } #endif return false; }
keydon2
Круто. Спасибо. Действительно Ваш вариант читается лучше, а длинные if встречаются практически везде.