Статический анализатор подталкивает писать чистый код

16 Апр 2024

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

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

Сразу перейдём к делу и начнём разбирать фрагмент кода на языке C, взятый из проекта iSulad.

/* container state to disk */
int container_state_to_disk(const container_t *cont)
{
    int ret = 0;

    if (cont == NULL) {
        return -1;
    }

    ret = container_save_container_state_config(cont);
    if (ret != 0) {
        return ret;
    }

    return ret;
}

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

Я наткнулся на этот фрагмент кода благодаря предупреждению анализатора PVS-Studio:

V523 The 'then' statement is equivalent to the subsequent code fragment. container_unix.c 878

Речь идёт об этом месте:

if (ret != 0) {
  return ret;
}
return ret;

Действительно, вне зависимости от того, какое значение имеет переменная ret, выполняется одно и то же действие.

Это не баг. Однако предупреждение анализатора заставляет задуматься о чистоте кода и, как вы сейчас увидите, "потянув за верёвочку", можно провести хороший рефакторинг.

Для начала сократим нижнюю часть кода до одного return:

/* container state to disk */
int container_state_to_disk(const container_t *cont)
{
    int ret = 0;

    if (cont == NULL) {
        return -1;
    }

    ret = container_save_container_state_config(cont);
    return ret;
}

Давайте не будем останавливаться на этом и продолжим улучшать код. Объявлять переменные в начале функции — это устаревший, вредный стиль написания кода. Он берёт своё начало от языков, где в начале функции требовалось объявлять все переменные. Но в их предварительном объявлении нет ничего замечательного, а наоборот, увеличивается шанс допустить ошибку. Например, начать использовать ещё неинициализированную переменную.

Рекомендуется объявлять переменную как можно ближе к месту её использования. Более того, лучше сразу её инициализировать в точке объявления.

Здесь переменная ret тоже инициализируется при объявлении. Но это "не та инициализация", она просто не имеет смысла. В теории, она может защитить от ошибки использования неинициализированных переменных. Однако лучше писать так, чтобы в принципе такой возможности не было. Для этого как раз и рекомендуют совместить объявление переменной с её инициализацией полезным значением.

В нашем случае это выглядит так:

/* container state to disk */
int container_state_to_disk(const container_t *cont)
{
    if (cont == NULL) {
        return -1;
    }

    int ret = container_save_container_state_config(cont);
    return ret;
}

При этом становится очевидно, что переменная ret вообще не нужна. Это повод избавиться от неё:

/* container state to disk */
int container_state_to_disk(const container_t *cont)
{
    if (cont == NULL) {
        return -1;
    }

    return container_save_container_state_config(cont);
}

Получилось упростить и сократить код. Good.

Здесь можно возразить, что предыдущий вариант кода был написан из расчёта его дальнейшего расширения. Ой, бросьте. Не надо писать код про запас. С большой вероятностью это расширение никогда не понадобится! А если такое произойдёт, то ничего сложного в добавлении кода нет.

Здесь уместна эта картинка:

Продолжим. Обратите внимание на комментарий к функции. В нём нет никакого смысла. Это калька с названия функции:

/* container state to disk */
int container_state_to_disk(const container_t *cont)

Комментарий ничего не поясняет, ничем не помогает. Это бессмысленная сущность, которую следует удалить.

int container_state_to_disk(const container_t *cont)
{
    if (cont == NULL) {
        return -1;
    }

    return container_save_container_state_config(cont);
}

Перед нами простая, красивая функция-прослойка, которая проверяет указатель и передаёт управление другой функции.

На этом всё? Оказывается, нет. Заглянем в ту — другую — вызываемую функцию:

static int container_save_container_state_config(const container_t *cont)
{
    int ret = 0;
    parser_error err = NULL;
    char *json_container_state = NULL;

    if (cont == NULL) {
        return -1;
    }

    container_state_lock(cont->state);
   ....
}

Ха! Оказывается, эта функция и сама имеет в себе проверку указателя, и ей можно безопасно передать NULL.

Соответственно код упрощается ещё больше:

int container_state_to_disk(const container_t *cont)
{
    return container_save_container_state_config(cont);
}

Что это значит? Что две эти функции — синонимы.

Это стоило написать в комментарии и не создавать бессмысленные проверки и действия в функции. Как мы выяснили, всё это был мусорный код, который ничего не делает и только занимает место.

Чего не хватает, так это приблизительно такого комментария:

/* На данный момент функция container_state_to_disk является
   синонимом container_save_container_state_config.
   Это изменится, когда будет реализовываться функционал .......
*/
int container_state_to_disk(const container_t *cont)
{
    return container_save_container_state_config(cont);
}

А если нет никакого "когда будет реализовываться"? Тогда текст программы можно упростить и сократить ещё больше с помощью вот такого макроса:

#define container_state_to_disk container_save_container_state_config

К этому макросу даже комментарий не нужен. И так понятно, что два эти названия являются синонимами.

P.S. Вообще, я не очень люблю макросы. Но без них в C жить тяжело, и аккуратное их использование не портит и не усложняет код. Поэтому я посчитал вполне уместным всё в итоге свести к макросу.

Пишите простой код. Не пишите избыточный код про запас. Спасибо за внимание.

#Cpp