Проверяя код проекта TDengine с помощью PVS-Studio, можно встретить канонические ошибки и опечатки. Многих из них можно избежать, если изначально аккуратно оформлять код, делать логику простой и избегать макросов. Давайте посмотрим на эти ошибки и подумаем, как можно повести рефакторинг кода так, чтобы им просто не было там места.
Код-колбаса
Проще всего то, что я имею в виду, может проиллюстрировать эта мемная картинка.
К сожалению, написание кода-колбасы свойственно не только Java программистам. Из-за нежелания разбить код на несколько строк и красиво его оформить, появляются длинные, плохо читаемые строки кода. Поскольку их сложно читать, меньше шанс, что при обзоре этого кода как самим программистом, так и его коллегами, там будут замечены ошибки.
Можно сказать, что длинные строчки кода притягивают к себе баги и опечатки. У меня нет какой-то статистики, но я очень часто обнаруживаю с помощью PVS-Studio баги именно в длинном, скучном или плохо оформленном коде.
Ошибка в TDengine
Рассмотрим классическую код-колбасу из проекта TDengine.
TDengine is an open source, high-performance, cloud native time-series database optimized for Internet of Things (IoT), Connected Cars, and Industrial IoT. It enables efficient, real-time data ingestion, processing, and monitoring of TB and even PB scale data per day, generated by billions of sensors and data collectors.
Вот он, в файле dmTransport.c (398).
Предупреждение PVS-Studio: V501 There are identical sub-expressions 'msgType == TDMT_VND_S3MIGRATE' to the left and to the right of the '||' operator. dmTransport.c 398
Действительно, если присмотреться, то можно заметить, что дважды проверяется одна и та же константа. Однако увидеть эту ошибку легко, только когда знаешь, что она есть. В реальности же прочитать этот код и заметить повтор достаточно сложно.
Рефакторинг
Попробуем улучшить код. Для начала разобьём его так, чтобы на каждой строке была одна проверка — опечатка сразу станет гораздо заметнее.
static bool rpcNoDelayMsg(tmsg_t msgType) {
if (msgType == TDMT_VND_FETCH_TTL_EXPIRED_TBS ||
msgType == TDMT_VND_S3MIGRATE ||
msgType == TDMT_VND_S3MIGRATE ||
msgType == TDMT_VND_QUERY_COMPACT_PROGRESS ||
msgType == TDMT_VND_DROP_TTL_TABLE) {
return true;
}
return false;
}Не будем останавливаться на достигнутом и оформим код табличным образом, выравнивая всё в столбики.
static bool rpcNoDelayMsg(tmsg_t msgType) {
if ( msgType == TDMT_VND_FETCH_TTL_EXPIRED_TBS
|| msgType == TDMT_VND_S3MIGRATE
|| msgType == TDMT_VND_S3MIGRATE
|| msgType == TDMT_VND_QUERY_COMPACT_PROGRESS
|| msgType == TDMT_VND_DROP_TTL_TABLE) {
return true;
}
return false;
}Теперь аномалия вообще как на ладони. И уже надо постараться, чтобы не заметить её.
Так как мы не знаем нюансов проверяемого проекта, то предположим, что повторная проверка просто лишняя, и на её месте не надо сравнивать msgType с ещё какой-то константой. Удаляем и получаем ещё немного рефакторинга.
static bool rpcNoDelayMsg(tmsg_t msgType) {
return msgType == TDMT_VND_FETCH_TTL_EXPIRED_TBS
|| msgType == TDMT_VND_S3MIGRATE
|| msgType == TDMT_VND_QUERY_COMPACT_PROGRESS
|| msgType == TDMT_VND_DROP_TTL_TABLE;
}Красота! Заодно тело функции сократилось с 5 строк (самый первый вариант) до 4.
Итог. Оформляя код красиво, мы:
- уменьшили вероятность опечатки;
- сделали код более коротким;
- сделали код более читаемым.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Breaking down bugs in TDengine to master refactoring, part 1: sausage code.
