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

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

В этот раз поговорим про написание кода методом Copy-Paste. С одной стороны, программисты знают, что копирование кода с последующей его модификацией провоцирует ошибки и опечатки. С другой — набирать каждый раз фрагмент кода, похожий на уже написанный, скучно и непродуктивно. Здесь важно соблюдать некий баланс, который сложно сформулировать и понимание которого приходит с опытом.

Я никогда не призывал полностью отказаться от копирования кода. Такой призыв звучит красиво, но нереалистично. Однако, увидев следующий код, думаю, вы согласитесь, что здесь Copy-Paste вышел за границу разумного.

Этот фрагмент в проекте TDengine навевает воспоминания о первой заметке про колбасу.

Однако сокращение длины строк кода в данном случае не помогает, и заметить ошибку всё также сложно.

int32_t mndSplitVgroup(SMnode *pMnode, SRpcMsg *pReq,
                       SDbObj *pDb, SVgObj *pVgroup)
{
  ....
  mInfo("vgId:%d, vgroup info after split, replica:"
        "%d hashrange:[%u, %u] vnode:0 dnode:%d",
        newVg1.vgId, newVg1.replica,
        newVg1.hashBegin, newVg1.hashEnd, newVg1.vnodeGid[0].dnodeId);
  for (int32_t i = 0; i < newVg1.replica; ++i) {
    mInfo("vgId:%d, vnode:%d dnode:%d",
          newVg1.vgId, i, newVg1.vnodeGid[i].dnodeId);
  }
  mInfo("vgId:%d, vgroup info after split, replica:%d hashrange:"
        "[%u, %u] vnode:0 dnode:%d", newVg2.vgId, newVg2.replica,
        newVg2.hashBegin, newVg2.hashEnd, newVg2.vnodeGid[0].dnodeId);
  for (int32_t i = 0; i < newVg1.replica; ++i) {
    mInfo("vgId:%d, vnode:%d dnode:%d",
          newVg2.vgId, i, newVg2.vnodeGid[i].dnodeId);
  }
  ....
}

Не видите ошибку? Ладно, не напрягайте глаза.

Предупреждение PVS-Studio:

V778 Two similar code fragments were found. Perhaps, this is a typo and 'newVg2' variable should be used instead of 'newVg1'. mndVgroup.c 3270

Ошибка во втором цикле:

for (int32_t i = 0; i < newVg1.replica; ++i) {

Он является копией первого цикла, в котором забыли заменить newVg1 на newVg2. Из-за этой ошибки будет распечатана не вся информация объекта newVg2 или, наоборот, произойдёт выход за границу массива.

Понятно, как появился баг. В начале программист написал код для распечатки значений объекта newVg1. Затем то же самое надо было сделать для объекта newVg2. И вот тут программист поленился, решив просто скопировать уже написанный фрагмент кода и заменить везде, где нужно, символ 1 на 2.

А мы уже знаем, что где один, два — там Фредди заберёт тебя :)

Проблема в том, что в этом блоке

mInfo("vgId:%d, vgroup info after split, replica:"
      "%d hashrange:[%u, %u] vnode:0 dnode:%d",
      newVg1.vgId, newVg1.replica,
      newVg1.hashBegin, newVg1.hashEnd, newVg1.vnodeGid[0].dnodeId);
for (int32_t i = 0; i < newVg1.replica; ++i) {
  mInfo("vgId:%d, vnode:%d dnode:%d",
        newVg1.vgId, i, newVg1.vnodeGid[i].dnodeId);
}

надо сделать восемь замен 1 на 2! Это много. Что-то легко пропустить, что, собственно, и произошло.

Экономия времени (лень) в данном случае выливается в ошибку, на поиск и устранение которой может быть потрачено на порядок больше времени. Сколько сэкономил времени программист? 5 минут.

Какие теоретические последствия? Предположим, у кого-то из пользователей произошёл выход за границу массива, и он отправил core dump. Переписка, открытие задачи на баг, поиск и исправление бага, проверка, пересборка, выдача обновлённой версии и так далее. Это могут быть часы. Причём рассмотрен оптимистичный сценарий, когда по сообщению пользователя сразу понятно, где именно скрылся баг. Иногда поиск подобных дефектов может растянуться на длительное время из-за попыток понять, что, как и почему падает у клиентов. Как следствие — потеря ресурсов и репутации.

В общем, Copy-Paste — опасная штука, как и нож. Причём мы все продолжим пользоваться как ножами, так и копированием кода. Вопрос только в том, где провести черту.

Сложно дать какую-то точную инструкцию, при превышении какого числа строк и количества замен надо не копировать блок, а писать функцию. Здесь нужна определённая интуиция.

Впрочем, в данном случае, сразу хочется создать функцию. Чувствуется, что многовато строк и замен. Проведём рефакторинг:

void PrintVGroupInfo(const SVgObj *vg)
{
  mInfo("vgId:%d, vgroup info after split, replica:"
        "%d hashrange:[%u, %u] vnode:0 dnode:%d",
        vg->vgId, vg->replica,
        vg->hashBegin, vg->hashEnd, vg->vnodeGid[0].dnodeId);
  for (int32_t i = 0; i < vg->replica; ++i) {
    mInfo("vgId:%d, vnode:%d dnode:%d",
          vg->vgId, i, vg->vnodeGid[i].dnodeId);
  }
}
....
int32_t mndSplitVgroup(SMnode *pMnode, SRpcMsg *pReq,
                       SDbObj *pDb, SVgObj *pVgroup)
{
  ....
  PrintVGroupInfo(&newVg1);
  PrintVGroupInfo(&newVg2);
  ....
}

Profit:

1. Для опечатки теперь просто нет места. Это самое главное.
2. Код печально длинной функции mndSplitVgroup стал короче. Да и в целом количество строк кода уменьшилось.

Вывод

Пишите красивый код. Как правило, красивый код — это более качественный и безопасный код. Кажется, в данном случае код с функцией более красив. Вот вам подсказка к действию.

Дополнительные ссылки:

1. Учимся рефакторить код на примере багов в TDengine, часть 1: про колбасу.
2. Учимся рефакторить код на примере багов в TDengine, часть 2: макрос, пожирающий стек.
3. Amnesia: The Dark Descent или как забыть поправить копипасту.
4. Проверка игрового движка qdEngine, часть вторая: упрощение C++ кода.
5. Статический анализатор подталкивает писать чистый код.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Breaking down bugs in TDengine to master refactoring, part 3: price of laziness.