Комментарии 75
Остальные два замечания — издержки Google C++ Style Guide. Это один из самых популярных стилей, поэтому он и использовался в примерах.
Я бы еще предложим немного расширить интерфейс обработки ошибок Accept'а (для консистентности).
Сейчас чтобы пользоваться функцией нужно:
- Знать с чем сравнивать (и как сравнивать) возвращаемое значение;
- Знать как в случае ошибки правильно вызывать коллбек, какие данные туда передавать, какие данные ожидают на той стороне.
Как-нибудь так:
const auto new_descriptor = Accept(listener, kAcceptTimeout);
if(is_invalid_descriptor(new_descriptor))
{
auto data = ProcessError(new_descriptor);
return on_data(data.descriptor, data.payload);
}
Позволю себе покритиковать полученный результат. Приведенный код все еще достаточно низкого качества для современного С++.
Во-первых, колбеки так уже никто не пишет. Желательно вместо std::string (*cb)(Descriptor, std::string) писать std::function<std::string(Descriptor, std::string)>. Однако тут тоже непонятно, что такое std::string в аргументе.
Если же мы говорим про асинхронный код, то так писать — это анахронизм. Лучше всего использовать future или корутины.
Далее, Descriptor — это скорее всего сетевой дескриптор. В таком случае не стоит использовать напрямую низкоуровневые примитивы в логике, а стоит их обернуть хотя бы в Socket. Смешивать высокоуровневую логику и низкоуровневые примитивы — признак плохого дизайна и мешанины.
Ну и напоследок, смешение стилей именования переменных: через _ и camelCase.
Ну и напоследок, смешение стилей именования переменных: через _ и camelCase.
Думаю, это кодстайл ClickHouse (возможно, не только он) там имена переменных с _, а у функций camelCase
Однако тут тоже непонятно, что такое std::string в аргументеНу так к чему полумеры? Можно же написать:
std::function<std::string(Descriptor connection, std::string name)>и сразу всё будет понятно.
ну, функция всего несколько строк, можно и так:
template<class OnData>
std::string Sample(Descriptor listener,
OnData &&onData){
...
}чтобы можно было с лямбдой вызывать. А тип колбэка проверить концептами
auto &name = p.first.secondЕще мой перфекционизм страдает от того, что first — 5 букв, second — 6, из-за чего выравнивание кода немного ломается. Мне больше нравится имена из haskell у пары: p.fst, p.snd — коротко и ясно.
Мне больше нравится имена из haskell у пары: p.fst, p.snd — коротко и ясно.
Глсн н нжн, вс и тк пнтн.
если уж так хочется читаемости без большого количества букв, то никто не мешает
Desciptor desk;
std::string payload;
std::tie(desk, payload) = Process(Options::kSync, new_descriptor);
return on_data(desk, payload);или вообще по модному
auto [desk, payload] = Process(Options::kSync, new_descriptor);
return on_data(desk, payload);
Маленький бонус — большинство компиляторов в C++ считают одиночные if без блока else холодным путём.
Что такое «холодный путь»? Термин не гуглится на просторах сети.
cold path | холодный путь — код на не критичном к производительности пути выполнения
Вот, например, GCC генерирует код с одиночным if'ом таким образом, что мы прыгаем только если у нас ошибка (условие выполнено)
https://godbolt.org/z/MET3vq
Мы можем явно сделать if "горячим" пометив его атрибутом [[likely]] и мы будем прыгать тогда когда ошибки нет (условие не выполнено), тогда год будет таким:
https://godbolt.org/z/51c6oT
Если бросается исключение, или вызывается функция из которой нет выхода, то этот путь становится для компилятора холодным
https://godbolt.org/z/bW78rv
Больше захардкоженных эвристик в gcc можно почитать здесь:
https://github.com/gcc-mirror/gcc/blob/master/gcc/predict.def
Что подтверждается: например, проверка указателя на 0 делает путь холодным https://godbolt.org/z/6PK7zs
Ну и C++20 не отстаёт:
Compute({.payload=payload, .timeout=1023});
Вы понимаете, что называя эмуляцию именованных параметров структурой «не отстает» и подавая ее как какое-то достижение C++20, вы закладываете мысль, что этот чудовищный костыль — в порядке вещей?
Сейчас это пойдет в массы, потом какой-нибудь гуру «паттерном» обзовет, а через 5 лет очередной study group сделает очередную выборку по github и придет к выводу, что всех все устраивает и нормальные именованные параметры можно и дальше не делать.
Примеры в статье смотрят не только C++ разработчики. Напихать C++ специфичного синтаксиса для экспертов, сделать метод шаблонным — это явно не те шаги, которые помогут усвоить материал.
* «Сужать скоуп до минимально возможного» — очень хороший пойнт, как раз то, что я просил написать в комментариях и что я забыл в статье.
* «Использовать общее решение» — да, но не уверен что это прям напрямую относится к читаемости кода и сложно назвать это простым приёмом.
* «Не использовать макросы, там где можно спокойно обойтись функциями.» — скорее стоит сформулировать как «Не использовать не рекомендуемые практики», чтобы не привязываться к конкретному языку программирования. Но тогда совет получается весьма абстрактным
В самом предложении http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0479r0.html есть несколько тестов
Зато хоть почувствовал и наглядно увидел повышение своего уровня за последние 2 года)
Оказывается, что плохо работает не ваш код, а функция sample от другого разработчика, которую вы используете.
Так где ошибка-то была? А то вы привели какой-то прямо каноничный пример:
antoshkka:
— Пришел в файл фиксить багу
— (через 5 минут) Ой, кто это писал, что за ужас, поправим стиль.
— (через 15 минут) мда, одним стилем не обойтись, надо капитально рефакторить
— (через 30 минут) вот, теперь хорошо *горд собой и постит статью на хабр*
Тимлид: «antoshkka, как там с тем багом, нашел проблему?»
antoshkka: "*искренне удивлен* Каким багом? *мучительный мыслительный процесс* А, с тем багом! Ой...."
Понятно, что хороший код лучше плохого, но не надо забывать о главной цели-то. И если код не такой уж и плохой (собственно, в исходном коде были косячки, ну магические числа, но ничего серьезного, что бы требовало рефакторинга ASAP, на мой взгляд), то может стоит сначала там тестов к нему написать, например, найти баг и пофиксить, и только потом за рефакторинг браться? Как говорится, Red-Green-Refactor.
и сразу всё будет понятно.
Это из комментария. Если читать статью вместе со всеми комментами, складывается довольно забавная картина. Вот автор взял чужой «плохой непонятный» текст и написал свой понятный ему в данный момент. Ему тут же накидали вариантов, понятных авторам этих вариантов. К чему это я? Давно живу, давно пишу. И прекрасно знаю одну вещь. Какие годы — через месяц-два, если «остыл» от задачи и переполз к другой, то старый текст начинает все больше и больше вызывать недоумение. Нет, разумеется, надо писать «хорошо». Константы в тексте — это, конечно, нонсенс. Но обо всем этом написано уже лет 40 назад все, что только можно. Рекомендации автора горячо поддерживаю. Но сильно опасаюсь, что их соблюдение не очень решит проблему на масштабе лет, ибо причина ее (проблемы), на мой непросвещенный взгляд, имеет более общий характер. Я допускаю, что есть выдающиеся люди, которые способны спустя, условно говоря, год снова, как ни в чем ни бывало, въехать в задачу и вновь почувствовать себя в ней как рыба в воде. Но полагаю, что большинство (и я в том числе) устроены иначе. «Холодный» старт всегда тяжелая штука. И свой собственный «правильно» написанный код спустя время воспринимается как изваянный полным придурком.
Ну...
- оригинальный код конечно не идеален :) но прочесть его как-то проблем не вызвало, ну и пройти на описание функций в современных средах вопрос секунд. Да, за безименные параметры и отсутсвие описания надо бить по рукам, но тоже не конец жизни.
- переименование cb на on_data ничего не дало, кроме доп вопросов. cb стандартное сокращение и практически сразу становится понятно, что она делает. а когда обработка ошибки вызвает on_data тут же возникают вопросы почему on_data?
- А почему главную проблему (название функции Sample) не исправили? с нее начинается процесс понимания, что происхдит ;-)
Мне кажется, что смешивать ассерты и кидание рантайм эксепции по одному условию — это демонстрация непонимания разницы между ассертами и рантайм ошибками. Ассерт нужно ставить там где нарушается контракт — баг — логическая ошибка. По другому, если после проваленного ассерта в коде не наблюдается УБ (в том числе вызывающем коде) то это не ассерт. Ещё по другому — рантайм ошибка/эксепция никогда не связанна с УБ, а наоборот штатная обработка внешних по отношению к программе невалидных данных.
Теперь смотрим ещё раз на ваш пример с ассертом — тип ошибки меняется из логической/баг в рантайм/штатная обработка/не баг в зависимости от дебажная это сборка или релизная. Я думаю теперь очевидно, что это какая-то ерунда, баг или есть или нет, в независимости от релиза/дебага.
ИНОГДА можно заменить ассерт на логическую эксепцию в релизе, но делать это нужно ОЧЕНЬ осторожно (привет не безопасный к эксепциям код) и только по очень специфическим причинам. Вообще в с/с++ отключение ассертов в релизе это преступление, за которое ...
Похоже из всех промышленных языков правильно из коробки обработка ошибок реализована только в расте.
Теперь смотрим ещё раз на ваш пример с ассертом — тип ошибки меняется из логической/баг в рантайм/штатная обработка/не баг в зависимости от дебажная это сборка или релизная.
Ну так UB — на то и UB, что его можно обрабатывать как угодно. В том числе по-разному в дебаге и в релизе!
Ну так на то оно и уб, что его как раз таки никак не обрабатывают, если его хоть как то обрабатывают то это уже не уб. Под обработкой я подразумеваю продолжение работы вызывающего кода, а не скажем аборт или очень глубокую раскрутку стека до уровня обработчика событий или границ таска/систем/языков/фреймворков.
Если у вас тотальная функция, то никакого уб по определению быть, не может. Если функция не тотальная то либо вы экономите каждый цикл и не делаете обработку ошибок ни в вызывающем ни в вызываемом коде и тогда уб, либо делаете обработку и тогда не уб. Проверка наличия ошибки не является обработкой и должна присутствовать всегда для не тотальных функций в идеале в виде контрактов которые ещё только обсуждаются.
Когда тип функции тотальная / не тотальная зависит от режима сборки это отличный способ отстреливать себе ноги по самые гланды.
Ну так на то оно и уб, что его как раз таки никак не обрабатывают, если его хоть как то обрабатывают то это уже не уб. Под обработкой я подразумеваю продолжение работы вызывающего кода, а не скажем аборт или очень глубокую раскрутку стека до уровня обработчика событий или границ таска/систем/языков/фреймворков.
Ну а я под обработкой подразумеваю любые действия, предпринятые железом. Включая аборт, раскрутку стека, аварийное завершение процесса, выключение компьютера… Попробуйте перечитать мой комментарий зная эту информацию.
Если у вас тотальная функция, то никакого уб по определению быть, не может.
А если у нас не тотальная функция, то UB возможно.
Когда тип функции тотальная / не тотальная зависит от режима сборки это отличный способ отстреливать себе ноги по самые гланды.
А он и не зависит. Фунцкия, содержащая assert, (почти) никогда не является тотальной, даже если в релизной сборке assert вырезается.
Фунцкия, содержащая assert, (почти) никогда не является тотальной, даже если в релизной сборке assert вырезается.
Ну вот тут мы и расходимся в понимании. Мне кажется вы слишком буквально/педантично воспринимаете сигнатуру функции как единственный критерий (не)тотальности.
Смотрите
// тут все понятно
int total(int x)
{
return x;
}
// тут тоже все понятно
double partial(int x)
{
assert(x >= 0);
return sqrt(x);
}
optinal<double> total(int x)
{
if(x < 0) return nullopt;
return sqrt(x);
}
varaint<double, errc> total(int x) //either
{
if(x < 0) return errc{1};
return sqrt(x);
}
// а вот тут уже не все понятно
double hz(int x)
{
assert(x >= 0);
0. if(x < 0) abort();
1. if(x < 0) throw logic_error("");
2. if(x < 0) throw runtime_error("");
return sqrt(x);
}
void caller()
{
x = ...;
// в таком варианте частичная
hz(x);
try
{
hz(x);
}
catch(runtime_error)
{
// в таком варианте сюрприз hz это тотальная функция в релизе и частичная в дебаге
// а эксепция использована для эмуляции either
// это собственно моя главная претензия, такого бардака писать не надо
}
catch(logic_error)
{
// за такое надо сразу карать
}
}
гдето далеко внизу колстека
void onEvent(event)
{
try
{
procces(event); // где то в глубине вызывает caller
}
catch(exception)
{
// это не всегда может закончиться нормально
// но если очень осторожно и очень надо можно попробовать
// и хорошенько оттестировать в ручную
}
}
Так вот ассерт + 0 = всегда ок, ассерт + 1 = опасно, но можно попробовать, ассерт + 2 = противоречие, остаться должен кто то один.
Если кидается runtime_error то подразумевается что ловить его можно и нужно, это просто иная форма variant/either. Если кидается logic_error то его точно нельзя ловить в непосредственном коллере, это просто сокрытие бага. И вот когда вы думаете ставить ассерт вы и должны решить это баг или нет. Если это не баг то это не ассерт, а если это баг то не кидайте рантайм эксепций или код ошибки.
Сегодня эксепции не часть сигнатуру и вы конечно можете говорить что я не прав, но что вы будете говорить когда/если примут предложение по статическим эксепциям где они часть сигнатуры? Надо смотреть не на формальности, а на то как оно используется фактически.
Еще раз:
ассерт + код ошибки/рантйайм эксепция = противоречие
ассерт = очень опасно
ассерт + логическая эксепция = опасно
ассерт + аборт / невыключаемый ассерт = ок
код ошибки/рантйайм эксепция = ок
Если в функции double hz(int x) написано assert(x >= 0) — это означает, что поведение этой функции для x<0 не определено. Что именно там происходит (аборт, logic_error, runtime_error, SIGSEG, повреждение памяти) — совершенно не важно, эту функцию попросту нельзя вызывать для отрицательных параметров!
Любая корректная программа никогда не вызывает hz для отрицательного x. А потому нет никакой разницы как этот самый assert(x >= 0) раскрывается: в корректной программе это всё равно лишняя проверка.
Мне кажется вы путаете причину и следствие. Вы пишете если ассерт, то нельзя. А должно быть если нельзя, то ассерт. На практике это как раз и есть проблема, вы подразумеваете что ассерт стоит правомерно, а я утверждаю что очень часто ассерт ставят не по делу. И если после ассерта стоит возврат ошибки или рантайм эксепция, то это явный признак того что ассерта там быть не должно вообще.
Насчёт уб, в моём примере хз проверяет предусловия, а это значит что уб не в хз, а вызывающем коде. Точнее не факт что там уб, факт что там баг. Когда хз превращается в тотальную функцию из частичной оно скрывает баг в вызывающем коде, в этом суть проблемы, поэтому очень даже важно что происходит когда детектиться уб/баг/предусловие. Чем ярче для пользователя реакция тем лучше для корректности, краш в таком случае очень хорошо (для корректности, не для пользователя).
Да, вы правы. Те случаи, когда assert стоит "не по делу", я обсуждать не вижу смысла.
И если после ассерта стоит возврат ошибки или рантайм эксепция, то это явный признак того что ассерта там быть не должно вообще.
Не понимаю что это за случаи такие.
Насчёт уб, в моём примере хз проверяет предусловия, а это значит что уб не в хз, а вызывающем коде.
Не вижу причин чтобы UB произошло снаружи.
Не понимаю что это за случаи такие.
Ну так пример из статьи + бесконечное количество в любом месте где бы я не работал, да что уж ханжить я и сам по молодости такое писал.
ASSERT(attempts > 0);
if (attempts <= 0) throw NegativeAttempts();
Не вижу причин чтобы UB произошло снаружи.
Ну так нашу частичную функцию вызвали с невалидным аргументом не для забавы ради, есть всего два варианта почему так произошло:
1. не проверили ввод, и тогда наша функция первая в цепочке вызовов, баг есть но УБ пока еще не произошло.
2. какой то код который уже отработал содержит баг — из валидного входа производит не верный результат и передает его на вход нашей функции, это и есть состояние что вызывающий код уже в состоянии УБ.
Вот это техника «защитного» программирования нацелена на 1 причину, но при этом очень сильно портит жизнь когда настоящая причина срабатывания ассерта номер 2.
Вот я уже даже перестал понимать, с чем у нас разногласие, чем больше общаемся тем больше соглашаемся. Скажите, вы сами в своем коде после ассерта будете использовать «защитное» программирование? И если да, то в каких случаях, но лучше пример.
На основе без преумеличения десятков примеров из опыта, я лично убежден (пока не предъявили контрпример), что «защитное» программирование это анти бест практис везде и всегда.
Ну рассуждать может и странно, но работать приходится с тем что есть, а не с тем что хочется и поэтому все эти бест практисы и пишутся — не от хорошей жизни.
А про пользу не тотальной функции вы уже сами ответили примером с корнем — эта функция фундаментально не тотальна для множества действительных, но если сузить домен (вход), а не расширить кодомен выход (возможно наоборот, я пока путаюсь) то все хорошо.
Самое главное что реализация не поменяется, поменяется только сигнатура, это и показывает практическую пользу этих функций. Подножки со стороны языка при попытках это выразить не отменяют пользу частичных функций, а за не имением ничего лучшего чем ассерты/контракты приходится спорить про то когда они уместны, а когда нет.
Я с вами вообщем то полностью согласен — в теории. Я даже пробовал реализовывать что то наподобие PositiveInt и использовать его в как в вашем примере с корнем — и я считаю что это НАМНОГО лучше чем обратный подход когда в качестве результата используются нуллабл.
На практике есть 2 фундаментальные проблемы с этим — мои коллеги не оценили сей подход от слова совсем. А вторая причина в том что с++ делает жизнь программиста невыносимой если нужно писать код на стыке подходов конверсии на конверсиях и конверсиями погоняет.
Первая проблема по сути не разрешима, люди (особенно те кто считает себя сеньором) уже привыкли писать частичные функции ну или в крайнем случае с расширением выхода и нет никакого способа в этом мире переубедить их. Надежда только на тех кто сейчас студенты и изучают фп.
Вторая проблема намного более техническая и вполне разрешима, нужно всего лишь переписать стандартную библиотеку в таком стиле, что автоматически для с++ означает — никогда, но есть надежда на новые языки или альтернативную версию стандартной библиотеки, что очень мало вероятно.
код, который не выбесит соседа
Кода который бы понравился всем не существует. Банально — есть программисты с немалым опытом, которые вообще не пишут комментарии. И считают, что так правильно. И есть программисты с таким же опытом, которые считают, что подробные комментарии обязательны.
Важных моментов два:
1. Перед приходом в команду попросить показать код (хотя бы через скайп, хотя бы пару файлов) — визуально оценить, понравится ли вам работать с таким кодом. Если код, который пишет команда вам сразу не нравится — то и идти работать, наверное, не стоит.
2. Во всех спорных моментах можно прийти к единому варианту, выработать единый стандарт комментариев, названий классов, свойств, методов в проекте. Можно (нужно) делать подробные ревью кода, выявлять непонятные места и править их. Но, все это требует времени. А его не будет, если разработка проходит в спешке. Соответственно лучше не работать в компаниях, где разработка проходит в постоянно горящих сроках.
P.S. Хороший код — это код в который не приходится заглядывать — который атомарно выполняет свою задачу и выполняет её хорошо и без ошибок :)
Выбор в моих краях к сожалению не так чтобы большой — верхняя граница это — сойдет если не принюхиваться, но проблема не в коде, а в людях / процессах. Сойдет если не принюхиваться качество кода это такой баланс между кого организация может захантить за рыночную стоимость и минимально приемлемым качеством продукта. Так что из моей практики в целом код в любом многолетнем не маленьком коммерческом продукте всегда на грани фола — так дешевле. Поэтому я больше стараюсь выяснить про процессы чем смотреть на код.
В итоге мы получаем ситуацию, когда делать правильно, как советует автор, не выгодно, если только ты не есть тем самым человеком, который писал это вчера или точно будет править это завтра. Ну или отвечаешь за весь релиз продукта и за все баги всех в команде.
форсировать базовые правила по написанию читаемого кодада так, что бы в результате у вас конфликт не получился и исправление было реализовано. На моей практике, очень небольшое количество людей адекватно воспринимает критику и эти люди в наименьшей степени нуждаются в ней, большая часть уходит в глухую оборону при ЛЮБОМ способе донесения идей отличных от тех, что уже поселились в голове.
превратилась в абсолютно трудно-читаемую.
if (result != -13)process(result, true, false, true);cb(data.second.first, data.second.second);более читаемо, чем
if (new_descriptor == kBadDescriptor)Process(Options::kSync, new_descriptor);on_data(data.descriptor, data.payload);
Чудеса, и как же автор такого кода прошел все круги вашего всесторонне-взвешенного интервью? Наверное, зато он сортирует массив как боженька, хотя и создавая 100500 переменных и магических чисел в процессе :)
Замечательная статья от Антона. От себя, я бы добавил еще правило:
Вместо использования множественных флагов — используйте enum'чики или классы флагов на их основе. В Qt есть замечательный класс QFlags, который как раз для таких дел.
Решается проблема:
1) Аргументы внезапно получают имена
2) Внезапно сокращается количество аргументов-флагов в прототипе функции
Прочти меня: код, который не выбесит соседа