Андрей Карпов

05 Мар 2025

Теги:

#Cpp #Knowledge

Учимся рефакторить код на примере багов в TDengine, часть 1: про колбасу

• Код-колбаса
• Ошибка в TDengine
• Рефакторинг

Проверяя код проекта TDengine с помощью PVS-Studio, можно встретить канонические ошибки и опечатки. Многих из них можно избежать, если изначально аккуратно оформлять код, делать логику простой и избегать макросов. Давайте посмотрим на эти ошибки и подумаем, как можно повести рефакторинг кода так, чтобы им просто не было там места.

Код-колбаса

Проще всего то, что я имею в виду, может проиллюстрировать эта мемная картинка.

К сожалению, написание кода-колбасы свойственно не только Java программистам. Из-за нежелания разбить код на несколько строк и красиво его оформить, появляются длинные, плохо читаемые строки кода. Поскольку их сложно читать, меньше шанс, что при обзоре этого кода как самим программистом, так и его коллегами, там будут замечены ошибки.

Можно сказать, что длинные строчки кода притягивают к себе баги и опечатки. У меня нет какой-то статистики, но я очень часто обнаруживаю с помощью PVS-Studio баги именно в длинном, скучном или плохо оформленном коде.

Ошибка в TDengine

Рассмотрим классическую код-колбасу из проекта TDengine.

Вот он, в файле 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.

Итог. Оформляя код красиво, мы:

• уменьшили вероятность опечатки;
• сделали код более коротким;
• сделали код более читаемым.

#Cpp #Knowledge

0

0

0

0

ПОДЕЛИТЬСЯ

Теги:

#Cpp #Knowledge

Присылаем лучшие статьи раз в месяц