60 антипаттернов для С++ программиста, часть 4 (совет 16 — 20)

Перед вами обновлённая коллекция вредных советов для C++ программистов, которая превратилась в целую электронную книгу. Всего их 60, и каждый сопровождается пояснением, почему на самом деле ему не стоит следовать. Всё будет одновременно и в шутку, и серьёзно. Как бы глупо ни смотрелся вредный совет, он не выдуман, а подсмотрен в реальном мире программирования.

Я буду публиковать советы по 5 штук, чтобы не утомить вас, так как мини-книга содержит много интересных отсылок на другие статьи, видео и т. д. Однако, если вам не терпится, здесь вы можете сразу перейти к её полному варианту: "60 антипаттернов для С++ программиста ^[1]". В любом случае желаю приятного чтения.

Вредный совет N16. sizeof(int) == sizeof(void *)

Размер указателя и int — это всегда 4 байта. Смело используйте это число. Число 4 смотрится намного изящнее, чем корявое выражение с оператором sizeof.

Размер int может быть очень даже разным. На многих популярных платформах размер int действительно 4 байта. Но многие – это не означает все! Существуют системы с различными моделями данных ^[2], где int может содержать и 8 байт, и 2 байта и даже 1 байт!

Формально про размер int можно сказать только следующее:

1 == sizeof(char) <=
  sizeof(short) <= sizeof(int) <= sizeof(long) <= sizeof(long long)

Указатель точно так же легко может отличаться от размера типа int и значения 4. Например, на большинстве 64-битных систем размер указателя составляет 8 байт, а типа int — 4 байта.

С этим связан достаточный распространённый паттерн 64-битной ошибки. В старых 32-битных программах иногда указатель сохраняли в переменные таких типов, как int/unsigned. При портировании таких программ на 64-битные системы возникают ошибки, так как при записи значения указателя в 32-битную переменную происходит потеря старших бит. См. главу "Упаковка указателей ^[3]" в курсе ^[3] по разработке 64-битных приложений.

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

1. Fundamental types ^[4].
2. What does the C++ standard state the size of int, long type to be? ^[5]

Вредный совет N17. Не проверяй, что вернула функция malloc

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

Упасть, если кончится память, допустимо игре. Это неприятно, но некритично. Ну если, конечно, в этот момент вы не участвуете в игровом чемпионате :).

Куда более грустно будет, если вы полдня делали проект в CAD-системе и приложение упало, когда для очередной операции потребовалось слишком много памяти. Одно дело – не дать выполнить какую-то операцию, и совсем другое – без предупреждения упасть. CAD и подобные системы должны продолжать работать, чтобы хотя бы дать возможность сохранить результат.

Несколько случаев, когда недопустимо писать код, просто падающий при нехватке памяти:

1. Встраиваемые системы. Там может быть "некуда падать" :). Многие встроенные программы должны продолжить выполнение в любом случае. Даже если нормально функционировать невозможно, программа должна отработать какой-то особый сценарий. Например, выключить оборудование, а только потом остановиться. В общем случае говорить про встроенное программное обеспечение и давать какие-то рекомендации невозможно. Уж очень эти системы и их назначение разнообразны. Главное, что игнорировать нехватку памяти и падать – это не вариант для таких систем;
2. Системы, где пользователь долго работает с каким-то проектом. Примеры: CAD-системы, базы данных, системы видеомонтажа. Падение в произвольный момент времени может привести к потере части работы или порче файлов проектов;
3. Библиотеки. Неизвестно, как и в каком проекте будет использоваться библиотека. Поэтому в них просто недопустимо игнорировать ошибки выделения памяти. Задача библиотеки – вернуть статус ошибки или бросить исключение. А уже пользовательскому приложению решать, что делать с возникшей ситуацией;
4. Прочее, про что я забыл или не подумал.

Данная тема во многом пересекается с моей статьёй "Четыре причины проверять, что вернула функция malloc ^[6]". Рекомендую. С ошибками выделения памяти не всё так просто и очевидно, как кажется на первый взгляд.

Вредный совет N18. Расширяй пространство std

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

Несмотря на то, что такая программа успешно компилируется и исполняется, модификация пространства имён std может привести к неопределённому поведению программы. То же самое касается и пространства posix.

Чтобы пояснить ситуацию, приведу часть документации PVS-Studio к диагностике V1061 ^[7], которая предназначена выявлять как раз такие недопустимые расширения пространства имён.

Содержимое пространства имен std определяется исключительно комитетом стандартизации. Стандарт запрещает добавлять в него:

• декларации переменных;
• декларации функций;
• декларации классов/структур/объединений;
• декларации перечислений;
• декларации шаблонов функций, классов и переменных (C++14).

Стандарт разрешает добавлять следующие специализации шаблонов, определённых в пространстве имен std, если они зависят хотя бы от одного определённого в программе типа (program-defined type):

• полная или частичная специализация шаблона класса;
• полная специализация шаблона функции (до C++20);
• полная или частичная специализация шаблона переменной, не лежащей в заголовочном файле <type_traits> (до C++20).

Однако специализации шаблонов, лежащих внутри классов или шаблонов классов, запрещены.

Наиболее частым вариантом, когда пользователь расширяет пространство имен std, является добавление своей перегрузки функции std::swap и полной/частичной специализации шаблона класса std::hash.

Рассмотрим неправильный фрагмент кода с добавлением перегрузки std::swap:

template <typename T>
class MyTemplateClass
{
  ....
};

class MyClass
{
  ....
};

namespace std
{
  template <typename T>
  void swap(MyTemplateClass<T> &a, MyTemplateClass<T> &b) noexcept // UB
  {
    ....
  }

  template <>
  void swap(MyClass &a, MyClass &b) noexcept // UB since C++20
  {
    ....
  };
}

Первый шаблон функции не является специализацией std::swap, и такая декларация ведёт к неопределённому поведению. Второй шаблон функции является специализацией, и до C++20 поведение программы определено. Однако в данном случае можно поступить иначе: можно вынести обе функции из пространства имен std и поместить их в то пространство имен, где определены классы:

template <typename T>
class MyTemplateClass
{
  ....
};

class MyClass
{
  ....
};

template <typename T>
void swap(MyTemplateClass<T> &a, MyTemplateClass<T> &b) noexcept
{
  ....
}

void swap(MyClass &a, MyClass &b) noexcept
{
  ....
};

Теперь, когда необходимо написать шаблон функции, который применяет функцию swap для двух объектов типа T, можно написать следующий код:

template <typename T>
void MyFunction(T& obj1, T& obj2)
{
  using std::swap; // make std::swap visible for overload resolution
  ....
  swap(obj1, obj2); // best match of 'swap' for objects of type T
  ....
}

Компилятор выберет нужную перегрузку функции на основе поиска с учётом аргументов (argument-dependent lookup, ADL): пользовательские функции swap для класса MyClass и для шаблона класса MyTemplateClass. И стандартную версию std::swap для остальных типов.

Разберём следующий пример со специализацией шаблона класса std::hash:

namespace Foo
{
    class Bar
    {
      ....
    };
}

namespace std
{
  template <>
  struct hash<Foo::Bar>
  {
    size_t operator()(const Foo::Bar &) const noexcept;
  };
}

С точки зрения стандарта этот код является валидным, и анализатор в этой ситуации не выдаёт предупреждение. Однако, начиная с C++11, можно и в этом случае поступить иначе, написав специализацию шаблона класса за пределами пространства имен std:

template <>
struct std::hash<Foo::Bar>
{
  size_t operator()(const Foo::Bar &) const noexcept;
};

В отличие от пространства имен std, стандарт C++ запрещает абсолютно любую модификацию пространства имён posix.

Дополнительная информация:

• Стандарт C++17 (working draft N4659), пункт 20.5.4.2.1
• Стандарт C++20 (working draft N4860), пункт 16.5.4.2.1

Вредный совет N19. Старая школа

Коллеги должны знать о вашем богатом опыте с языком C. Не стесняйтесь демонстрировать им в вашем C++ проекте свои умелые навыки ручного управления памятью и longjmp.

Другая вариация этого вредного совета: умные указатели и прочее RAII от лукавого, всеми ресурсами надо управлять вручную, это делает код простым и понятным.

Нет основания отказываться от умных указателей и городить сложные конструкции при работе с памятью. Умные указатели в С++ не требуют дополнительного процессорного времени, это не сборка мусора ^[8]. При этом код с использованием умных указателей становится короче и проще, что дополнительно снижает вероятность допустить ошибку.

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

Примечание. Я рассматриваю в примерах выделение и освобождение памяти. На самом деле, это более широкая тема ручного управления ресурсами. Вместо malloc вполне можно подставить, например, fopen.

int Foo()
{
  float *buf = (float *)malloc(ARRAY_SIZE * sizeof(float));
  if (buf == NULL)
    return STATUS_ERROR_ALLOCATE;
  int status = Go(buf);
  free(buf);
  return status;
}

Этот код прост и понятен. Функция выделят память для каких-то нужд, использует её и затем освобождает. Дополнительно приходится проверять, смогла ли функция malloc выделить память. Почему эта проверка обязательно необходима мы разбирали в главе N17.

Теперь представим, что нам требуется выполнить операции с двумя разными буферами. Код сразу начинает пухнуть, так как при ошибки очередном выделении памяти, нужно позаботиться о предыдущем буфере. Плюс теперь нужно учитывать, что вернула функция Go_1.

int Foo()
{
  float *buf_1 = (float *)malloc(ARRAY_SIZE_1 * sizeof(float));
  if (buf_1 == NULL)
    return STATUS_ERROR_ALLOCATE;
  int status = Go_1(buf_1);
  if (status != STATUS_OK)
  {
    free(buf_1);
    return status;
  }
  float *buf_2 = (float *)malloc(ARRAY_SIZE_2 * sizeof(float));
  if (buf_2 == NULL)
  {
    free(buf_1);
    return STATUS_ERROR_ALLOCATE;
  }
  status = Go_2(buf_1, buf_2);
  free(buf_1);
  free(buf_2);
  return status;
}

Дальше — хуже. Размер кода растёт нелинейно. При трёх буферах:

int Foo()
{
  float *buf_1 = (float *)malloc(ARRAY_SIZE_1 * sizeof(float));
  if (buf_1 == NULL)
    return STATUS_ERROR_ALLOCATE;
  int status = Go_1(buf_1);
  if (status != STATUS_OK)
  {
    free(buf_1);
    return status;
  }
  float *buf_2 = (float *)malloc(ARRAY_SIZE_2 * sizeof(float));
  if (buf_2 == NULL)
  {
    free(buf_1);
    return STATUS_ERROR_ALLOCATE;
  }
  status = Go_2(buf_1, buf_2);
  if (status != STATUS_OK)
  {
    free(buf_1);
    free(buf_2);
    return status;
  }
  float *buf_3 = (float *)malloc(ARRAY_SIZE_3 * sizeof(float));
  if (buf_3 == NULL)
  {
    free(buf_1);
    free(buf_2);
    return STATUS_ERROR_ALLOCATE;
  }
  status = Go_3(buf_1, buf_2, buf_3);
  free(buf_1);
  free(buf_2);
  free(buf_3);
  return status;
}

Что интересно, сложность кода по-прежнему низкая. Его легко писать и читать. Но вместе с тем чувствуется, что это какой-то неправильный путь. Больше половины кода функции не делает что-то полезное, а занимается проверкой статусов и выделением/освобождением памяти. Вот этим и плохо ручное управление памятью. Много нужного, но не относящегося к делу кода.

И хотя код, как я сказал, несложен, с ростом его размера всё проще допустить ошибку. Например, можно при досрочном выходе из функции забыть освободить какой-то указатель и получить утечку памяти. И такое мы действительно встречаем ^[9] в коде различных проектов, когда проверяем их с помощью PVS-Studio. Вот, например, фрагмент кода из проекта PMDK:

static enum pocli_ret
pocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp)
{
  char *input = strdup(in);
  if (!input)
    return POCLI_ERR_MALLOC;

  if (!oidp)
    return POCLI_ERR_PARS;
  ....
}

Функция strdup создаёт копию строки в буфере, который затем должен где-то быть освобождён с помощью функции free. Здесь же в случае, если аргумент oidp является нулевым указателем, произойдёт утечка памяти. Корректный код должен быть таким:

char *input = strdup(in);
if (!input)
  return POCLI_ERR_MALLOC;

if (!oidp)
{
  free(input);
  return POCLI_ERR_PARS;
}

Или нужно перенести проверку аргумента в начало функции:

if (!oidp)
  return POCLI_ERR_PARS;

char *input = strdup(in);
if (!input)
  return POCLI_ERR_MALLOC;

В любом случае перед нами классическая ошибка в коде с ручным управлением памяти.

Вернёмся к нашему синтетическому коду с тремя буферами. Можно как-то сделать проще? Да, для этого используется паттерн с одной точкой выхода и операторами goto.

int Foo()
{
  float *buf_1 = NULL;
  float *buf_2 = NULL;
  float *buf_3 = NULL;
  int status;

  buf_1 = (float *)malloc(ARRAY_SIZE_1 * sizeof(float));
  if (buf_1 == NULL)
  {
    status = STATUS_ERROR_ALLOCATE;
    goto end;
  }
  status = Go_1(buf_1);
  if (status != STATUS_OK)
    goto end;
  buf_2 = (float *)malloc(ARRAY_SIZE_2 * sizeof(float));
  if (buf_2 == NULL)
  {
    status = STATUS_ERROR_ALLOCATE;
    goto end;
  }
  status = Go_2(buf_1, buf_2);
  if (status != STATUS_OK)
  {
    status = STATUS_ERROR_ALLOCATE;
    goto end;
  }
  buf_3 = (float *)malloc(ARRAY_SIZE_3 * sizeof(float));
  if (buf_3 == NULL)
  {
    status = STATUS_ERROR_ALLOCATE;
    goto end;
  }
  status = Go_3(buf_1, buf_2, buf_3);
end:
  free(buf_1);
  free(buf_2);
  free(buf_3);
  return status;
}

Это лучше, и так часто пишут программисты на C. Я не могу назвать такой код хорошим и красивым, но что делать. Ручное управление ресурсами в любом случае страшненькое...

Кстати, некоторые компиляторы поддерживают специальное расширение языка C, которое помогает упростить жизнь. Можно использовать конструкции вида:

void free_int(int **i) {
  free(*i);
}

int main(void) {
  __attribute__((cleanup (free_int))) int *a = malloc(sizeof *a);
  *a = 42;
} // No memory leak, free_int is called when a goes out of scope

Подробнее про эту магию можно почитать здесь: RAII in C: cleanup gcc compiler extension ^[10].

Вернёмся к вредному совету. Беда в том, что некоторые программисты продолжают использовать этот стиль ручного управления ресурсами в С++ коде, хотя в этом нет никакого смысла! Не делайте так. С++ позволяет сделать код простым и коротким.

Можно использовать контейнеры, такие как std::vector. Но, даже если нам нужен именно массив байт, выделенный с помощью оператора new [], всё равно код можно сделать на порядок лучше.

int Foo()
{
  std::unique_ptr<float[]> buf_1 (new float[ARRAY_SIZE_1]);
  if (int status = Go_1(buf_1); status != STATUS_OK)
    return status;

  std::unique_ptr<float[]> buf_2(new float[ARRAY_SIZE_2]);
  if (int status = Go_2(buf_1, buf_2); status != STATUS_OK)
    return status;

  std::unique_ptr<float[]> buf_3(new float[ARRAY_SIZE_3]);
  reutrn Go_3(buf_1, buf_2, buf_3);
}

Красота! Проверять результат вызова оператора new [] не нужно, так как в случае ошибки создания буфера будет сгенерировано исключение. Буферы сами освобождаются, если возникают исключения или при штатном завершении функции.

Так какой же смысл писать по старинке в С++? Никакого. Тогда почему можно встретить такой код? Я думаю, это может происходить вследствие следующих причин.

Первый вариант. Человек делает это просто по привычке. Он не хочет изучать что-то новое, переучиваться. Фактически он пишет код на C с использованием каких-то возможностей C++. Грустный вариант, и непонятно, что тут посоветовать.

Второй вариант. Перед вами С++ код, который когда-то был кодом на C. Его немного изменили, но не переписали и не отрефакторили. Т.е. просто заменили malloc на new, а free на delete. Такой код можно легко распознать по двум артефактам.

Во-первых, в нём присутствуют вот такие проверки-атавизмы:

in_audio_ = new int16_t[AUDIO_BUFFER_SIZE_W16];
if (in_audio_ == NULL) {
  return -1;
}

Нет смысла проверять указатель на равенство NULL, так как в случае ошибки выделения памяти будет сгенерировано исключение типа std::bad_alloc. Очень, очень частный атавизм ^[11]. Конечно, существует new(std::nothrow) ^[12], но это не наш случай.

Во-вторых, там часто есть ошибка, которая заключается в том, что память выделяется с помощью оператора new [], а освобождается с помощью delete. Хотя правильно использовать delete []. См. "Почему в С++ массивы нужно удалять через delete[] ^[13]". Пример:

char *poke_data = new char [length + 2*sizeof(int)];
....
delete poke_data;

Третий вариант. Боязнь дополнительных накладных расходов. Необоснованный страх. Да, у умных указателей иногда могут быть незначительные накладные расходы по сравнению с простыми указателями. Однако следует принять в расчёт:

1. Возможные накладные расходы от умных указателей пренебрежимо малы по сравнению с относительно медленными операциями выделения и освобождения памяти. Если нужна максимальная скорость, то стоит думать, как уменьшить количество операций выделения/освобождения памяти, а не над тем: использовать умный указатель или контролировать указатели вручную. Ещё вариант — написать собственный аллокатор;
2. Простота, надёжность и безопасность кода, использующего умные указатели, на мой взгляд, однозначно перевешивает дополнительные накладные расходы (которых, кстати, может и не быть).

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

• Memory and Performance Overhead of Smart Pointers ^[14].
• How much is the overhead of smart pointers compared to normal pointers in C++? ^[15]

Четвёртый вариант. Программисты просто не осведомлены о том, как можно использовать тот же std::unique_ptr. Условно, они рассуждают так:

Хорошо, есть std::unique_ptr. Он умеет контролировать указатель на объект. Но мне-то ещё нужно работать с массивами объектов. А ещё есть дескрипторы файлов. Местами я вообще вынужден по-прежнему использовать malloc/realloc. Для всего этого unique_ptr не подходит. Так что проще для единообразия продолжать везде управлять ресурсами вручную.

Всё, что описано, очень даже можно контролировать с помощью std::unique_ptr ^[16].

// Работа с массивами:
std::unique_ptr<T[]> ptr(new T[count]);

// Работа с файлами:
std::unique_ptr<FILE, int(*)(FILE*)> f(fopen("a.txt", "r"), &fclose);

// Работа с malloc:
struct free_delete
{
  void operator()(void* x) { free(x); }
};
....
std::unique_ptr<int, free_delete> up((int*)malloc(sizeof(int)));

На этом всё. Надеюсь, если у вас оставались сомнения в умных указателях, я их развеял.

P.S. Я ничего не написал про longjmp ^[17]. И не вижу смысла. В C++ для тех же целей следует использовать исключения.

Вредный совет N20. Компактный код

Используйте как можно меньше фигурных скобок и переносов строк, старайтесь писать условные конструкции в одну строку. Так код будет быстрее компилироваться и занимать меньше места.

Что код будет короче – это бесспорно. Бесспорно и то, что в нём будет больше ошибок.

"Сжатый код" сложнее читать, а значит, с большей вероятностью опечатки не будут замечены ни автором кода, ни коллегами при обзоре кода. Хотите какой-нибудь proof? Легко!

Однажды пользователь написал нам о том, что анализатор PVS-Studio выдаёт странные ложные срабатывания на условие. И прикрепил вот такую картинку:

Видите ошибку в коде? Скорее всего, нет. А почему? А потому, что перед нами большое сложное выражение, написанное в одну строчку. Человеку сложно прочитать и осознать этот код. Скорее всего, вы и не стали пробовать разобраться, а сразу продолжили читать статью :).

А вот анализатор не поленился и совершенно справедливо указывает на аномалию: часть подвыражений всегда истинны или ложны. Давайте проведём рефакторинг кода:

if (!((ch >= 0x0FF10) && (ch <= 0x0FF19)) ||
     ((ch >= 0x0FF21) && (ch <= 0x0FF3A)) ||
     ((ch >= 0x0FF41) && (ch <= 0x0FF5A)))

Теперь намного легче заметить, что логический оператор "не" (!) применяется только к первому подвыражению. В общем, здесь не хватает ещё одних скобочек. Эта ошибка, а также то, почему анализатор выдал предупреждения, разбирается в статье "Как PVS-Studio оказался внимательнее, чем три с половиной программиста ^[18]".

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

Давайте разберём эту тему подробнее на другом примере. Возьмём фрагмент кода из проекта ReactOS, в котором я нашёл ошибку благодаря предупреждению PVS-Studio: V560 A part of conditional expression is always true: 10035L.

void adns__querysend_tcp(adns_query qu, struct timeval now) {
  ...
  if (!(errno == EAGAIN || EWOULDBLOCK || 
        errno == EINTR || errno == ENOSPC ||
        errno == ENOBUFS || errno == ENOMEM)) {
  ...
}

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

Причина, почему мы пропускаем такие ошибки, в том, что условия плохо отформатированы и не хочется внимательно их читать, это требует усилий. Мы надеемся, что раз проверки однотипные, то всё хорошо, и автор кода не допустил ошибок в условии.

Одним из способов борьбы с опечатками является "табличное" оформление кода.

Для читателей, поленившихся найти ошибку, скажу, что в одном месте пропущено "errno ==". В результате условие всегда истинно, так как константа EWOULDBLOCK равна 10035. Корректный код:

if (!(errno == EAGAIN || errno == EWOULDBLOCK || 
      errno == EINTR || errno == ENOSPC ||
      errno == ENOBUFS || errno == ENOMEM)) {

Теперь рассмотрим, как лучше провести рефакторинг этого фрагмента. Для начала я приведу код, оформленный самым простым "табличным" способом. Мне он не нравится.

if (!(errno == EAGAIN  || EWOULDBLOCK     || 
      errno == EINTR   || errno == ENOSPC ||
      errno == ENOBUFS || errno == ENOMEM)) {

Стало лучше, но ненамного. Такой стиль оформления мне не нравится по двум причинам:

1. Ошибка по-прежнему не очень заметна;
2. Приходится вставлять большое количество пробелов для выравнивания.

Поэтому надо сделать два усовершенствования в оформлении кода. Первое — не больше одного сравнения на строку, тогда ошибку легко заметить. Смотрите, ошибка стала больше бросаться в глаза:

a == 1 &&
b == 2 &&
c      &&
d == 3 &&

Второе — рационально писать операторы &&, || и т.д., не справа, а слева.

Обратите внимание, как много работы для написания пробелов:

x == a          &&
y == bbbbb      &&
z == cccccccccc &&

А вот так работы намного меньше:

   x == a
&& y == bbbbb
&& z == cccccccccc

Выглядит код немного необычно, но к этому быстро привыкаешь.

Объединим это всё вместе и напишем в новом стиле код, приведённый в начале:

if (!(   errno == EAGAIN
      || EWOULDBLOCK
      || errno == EINTR
      || errno == ENOSPC
      || errno == ENOBUFS
      || errno == ENOMEM)) {

Да, код стал занимать больше строк кода, но зато ошибка стала намного заметнее.

Согласен, смотрится код непривычно. Тем не менее, я рекомендую именно этот подход. Я пользуюсь им много лет и весьма доволен, поэтому с уверенностью рекомендую его всем читателям.

То, что код стал длиннее, я вообще не считаю проблемой. Я даже написал бы как-то так:

const bool error =    errno == EAGAIN
                   || errno == EWOULDBLOCK
                   || errno == EINTR
                   || errno == ENOSPC
                   || errno == ENOBUFS
                   || errno == ENOMEM;
if (!error) {

Кто-то ворчит, что это длинно и загромождает код? Согласен. Так давайте вынесем это в функцию!

static bool IsInterestingError(int errno)
{
  return    errno == EAGAIN
         || errno == EWOULDBLOCK
         || errno == EINTR
         || errno == ENOSPC
         || errno == ENOBUFS
         || errno == ENOMEM;
}
....
if (!IsInterestingError(errno)) {

Может показаться, что я сгущаю краски и что я слишком перфекционист, однако ошибки в сложных выражениях очень распространены. Я бы не вспомнил о них, если бы они мне постоянно не попадались: эти ошибки повсюду и плохо заметны.

Вот ещё один пример из проекта WinDjView:

inline bool IsValidChar(int c)
{
  return c == 0x9 || 0xA || c == 0xD || 
         c >= 0x20 && c <= 0xD7FF ||
         c >= 0xE000 && c <= 0xFFFD || 
         c >= 0x10000 && c <= 0x10FFFF;
}

В функции всего несколько строк, и всё равно в неё закралась ошибка. Функция всегда возвращает true. Вся беда в том, что она плохо оформлена, и многие годы её ленятся читать и не замечают там ошибку.

Давайте отрефакторим код в "табличном" стиле, и я бы еще скобочки добавил:

inline bool IsValidChar(int c)
{
  return
       c == 0x9
    || 0xA
    || c == 0xD
    || (c >= 0x20    && c <= 0xD7FF)
    || (c >= 0xE000  && c <= 0xFFFD)
    || (c >= 0x10000 && c <= 0x10FFFF);
}

Необязательно форматировать код именно так, как я предлагаю. Смысл этой заметки — привлечь ваше внимание к опечаткам в "хаотичном коде". Придавая коду "табличный" вид, можно избежать множества глупых опечаток, и это замечательно. Надеюсь, кому-то эта заметка принесёт пользу.

Ложка дёгтя. Я честный программист, и поэтому должен упомянуть, что иногда форматирование "таблицей" может пойти во вред. Вот один из примеров:

inline 
void elxLuminocity(const PixelRGBi& iPixel,
                   LuminanceCell< PixelRGBi >& oCell)
{
  oCell._luminance = 2220*iPixel._red +
                     7067*iPixel._blue +
                     0713*iPixel._green;
  oCell._pixel = iPixel;
}

Это проект eLynx SDK. Программист хотел выровнять код, поэтому перед 713 дописал 0. К сожалению, программист не учёл, что 0 в начале числа означает, что число будет представлено в восьмеричном формате.

Массив строк. Надеюсь, концепция форматирования "таблицей" понятна, но давайте закрепим тему. С этой целью рассмотрим пример, которым я продемонстрирую, что табличное форматирование можно применять не только к условиям, а к совершенно разным конструкциям языка.

Фрагмент взят из проекта Asterisk. Ошибка выявляется PVS-Studio диагностикой: V653 A suspicious string consisting of two parts is used for array initialization. It is possible that a comma is missing. Consider inspecting this literal: "KW_INCLUDES" "KW_JUMP".

static char *token_equivs1[] =
{
  ....
  "KW_IF",
  "KW_IGNOREPAT",
  "KW_INCLUDES"
  "KW_JUMP",
  "KW_MACRO",
  "KW_PATTERN",
  ....
};

Опечатка — забыта одна запятая. В результате две различные по смыслу строки соединяются в одну, т. е. на самом деле здесь написано:

  ....
  "KW_INCLUDESKW_JUMP",
  ....

Ошибку можно было бы избежать, выравнивая код таблицей. Тогда, если запятая будет пропущена, это будет легко заметить.

static char *token_equivs1[] =
{
  ....
  "KW_IF"        ,
  "KW_IGNOREPAT" ,
  "KW_INCLUDES"  ,
  "KW_JUMP"      ,
  "KW_MACRO"     ,
  "KW_PATTERN"   ,
  ....
};

Как и в прошлый раз, обращаю внимание, что если мы ставим разделитель справа (в данном случае это запятая), то приходится добавлять массу пробелов, что неудобно. Особенно неудобно, если появляется новая длинная строка/выражение: придётся переформатировать всю таблицу.

Поэтому я вновь рекомендую оформлять код так:

static char *token_equivs1[] =
{
  ....
  , "KW_IF"
  , "KW_IGNOREPAT"
  , "KW_INCLUDES"
  , "KW_JUMP"
  , "KW_MACRO"
  , "KW_PATTERN"
  ....
};

Теперь очень легко заметить недостающую запятую, и нет необходимости писать много пробелов — код красивый и понятный. Быть может, такое оформление непривычно, но к нему быстро привыкаешь, попробуйте.

И под конец короткий лозунг. Как правило, красивый код — это правильный код.

Об этой мини-книге

Автор: Карпов Андрей Николаевич. E-Mail: karpov [@] viva64.com.

Более 15 лет занимается темой статического анализа кода и качества программного обеспечения. Автор большого количества статей, посвящённых написанию качественного кода на языке C++. С 2011 по 2021 год удостаивался награды Microsoft MVP в номинации Developer Technologies. Один из основателей проекта PVS-Studio. Долгое время являлся CTO компании и занимался разработкой С++ ядра анализатора. Основная деятельность на данный момент — управление командами, обучение сотрудников и DevRel активность.

Ссылки на полный текст:

• RU — 60 антипаттернов для С++ программиста ^[1]
• EN — 60 terrible tips for a C++ developer ^[19]

Подписывайтесь ^[20] на ежемесячную рассылку, чтобы не пропустить другие публикации автора и его коллег.