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