Учимся рефакторить код на примере багов в 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.