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

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

![1242_TDengine_refactoring_3_ru/image1.png](https://import.viva64.com/docx/blog/1242_TDengine_refactoring_3_ru/image1.png)

В этот раз поговорим про написание кода [методом Copy\-Paste](https://pvs-studio.ru/ru/blog/terms/0068/)\. С одной стороны, программисты знают, что копирование кода с последующей его модификацией провоцирует ошибки и опечатки\. С другой — набирать каждый раз фрагмент кода, похожий на уже написанный, скучно и непродуктивно\. Здесь важно соблюдать некий баланс, который сложно сформулировать и понимание которого приходит с опытом\.

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

Этот [фрагмент](https://github.com/taosdata/TDengine/blob/100136fa4e292d1f3318e379ee62a3eec3b29e52/source/dnode/mnode/impl/src/mndVgroup.c#L3270) в проекте TDengine навевает воспоминания о [первой заметке про колбасу](https://pvs-studio.ru/ru/blog/posts/cpp/1230/)\.

![1242_TDengine_refactoring_3_ru/image2.png](https://import.viva64.com/docx/blog/1242_TDengine_refactoring_3_ru/image2.png)

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v778/) Two similar code fragments were found\. Perhaps, this is a typo and 'newVg2' variable should be used instead of 'newVg1'\. mndVgroup\.c 3270

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

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

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

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

А мы уже знаем, что где [один, два — там Фредди заберёт тебя](https://pvs-studio.ru/ru/blog/posts/cpp/0713/) :\)

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

```cpp
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 — опасная штука, как и нож\. Причём мы все продолжим пользоваться как ножами, так и копированием кода\. Вопрос только в том, где провести черту\.

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

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

```cpp
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. Для опечатки теперь просто нет места\. Это самое главное\.
1. Код печально длинной функции `mndSplitVgroup` стал короче\. Да и в целом количество строк кода уменьшилось\.

**Вывод** 

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

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

1. [Учимся рефакторить код на примере багов в TDengine, часть 1: про колбасу](https://pvs-studio.ru/ru/blog/posts/cpp/1230/)\.
1. [Учимся рефакторить код на примере багов в TDengine, часть 2: макрос, пожирающий стек](https://pvs-studio.ru/ru/blog/posts/cpp/1238/)\.
1. [Amnesia: The Dark Descent или как забыть поправить копипасту](https://pvs-studio.ru/ru/blog/posts/cpp/0769/)\.
1. [Проверка игрового движка qdEngine, часть вторая: упрощение C\+\+ кода](https://pvs-studio.ru/ru/blog/posts/cpp/1121/)\.
1. [Статический анализатор подталкивает писать чистый код](https://pvs-studio.ru/ru/blog/posts/cpp/1115/)\.