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

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

Сразу перейдём к делу и начнём разбирать фрагмент кода на языке 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 жить тяжело, и аккуратное их использование не портит и не усложняет код. Поэтому я посчитал вполне уместным всё в итоге свести к макросу.

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Static analyzer nudges you to write clean code.