60 антипаттернов для С++ программиста, часть 12 (совет 56 — 60)
Перед вами обновлённая коллекция вредных советов для C++ программистов, которая превратилась в целую электронную книгу. Всего их 60, и каждый сопровождается пояснением, почему на самом деле ему не стоит следовать. Всё будет одновременно и в шутку, и серьёзно. Как бы глупо ни смотрелся вредный совет, он не выдуман, а подсмотрен в реальном мире программирования.
Я буду публиковать советы по 5 штук, чтобы не утомить вас, так как мини-книга содержит много интересных отсылок на другие статьи, видео и т. д. Однако, если вам не терпится, здесь вы можете сразу перейти к её полному варианту:»60 антипаттернов для С++ программиста». В любом случае желаю приятного чтения.
Вредный совет N56. Больше классов!
Всё, что может быть представлено классом, должно быть им представлено.
Мне кажется, подобный максимализм возникает в момент нездоровой увлечённости ООП. Как кто-то сказал: «Когда у вас в руке молоток, всё становится похожим на гвозди».
Отсылаю вас к докладу Джэка Дидриха «Перестаньте писать классы». Там про язык Python, но не суть важно.
Или можно познакомиться с переводом этого доклада, оформленного в виде статьи.
Вредный совет N57. Чтение книг уже неактуально
Чтение фундаментальных книг по С++ — пустая трата времени. Достаточно обучающих видео, во всём остальном поможет Google.
Я большой сторонник чтения книг по программированию. При этом я не отрицаю пользу от обучающих видео и других способов обучения. Я сам могу с удовольствием посмотреть какой-то доклад с C++ конференции.
Однако у обучающих видео есть два недостатка:
- Они не дают остановиться и обдумать увиденное и услышанное. Можно поставить на паузу, отмотать, но всё это не так удобно и естественно, как при чтении книги. Изучая C++ в книгах, я нередко перечитывал абзацы или повторно медленно изучал код примеров. Делал паузу, чтобы обдумать. В общем, я о вдумчивом чтении. В случае видео всё это тоже можно делать, но как-то так не получается. Ты смотришь и смотришь дальше, даже если что-то было не совсем понятно. Иногда ещё и ускоренно.
- Обучающие видео не дают такой же целостной картины, как книги. Я не представляю, как можно удобоваримо запихать в видео такой же объём знаний, которые, например, изложены на 1200 страницах книги Бьёрна Страуструпа «Язык программирования C++» (4 издание).
На мой взгляд, видео может дополнять полученные знания. Они полезны, когда ложатся поверх фундамента знаний, полученного из книг. В противном случае они формируют неглубокую разорванную картину знаний. Кажется, что человек разбирается в программировании, но такие «лоскутные» знания быстро выйдут боком и ему, и работодателю.
Я, кстати, встречал таких «программистов». Первое поверхностное впечатление — они очень крутые. Они могут быстро что-то создать на хакатоне, используя какую-то модную AI-библиотеку, или удивить какой-то немереной красотой, использовав где-то игровой движок. Это кажется какой-то магией, особенно если ты сам не работал с этими библиотеками. Вот только всё это рано или поздно разбивается о незнание каких-то базовых алгоритмов или особенностей языка программирования.
А в чём проблема, если что-то человек не знает, но тем не менее в целом что-то делает? В неэффективности и ограниченности возможностей. Из-за незнания человек может использовать странные крайне неоптимальные подходы. Или может программировать методом поиска готовых решений в Google или на Stack Overflow. Но «нагуглить» можно не всё, и рано или поздно человек просто не сможет решить стоящие перед ним задачи.
В общем, я подвожу вас к относительно свежей проблеме «Google-программистов». Доступность обучающих видео «как сделать X» и готовых проектов/решений в интернете порождает программистов, которые не умеют решать задачи самостоятельно. А ведь рано или поздно они сталкиваются с такими задачами. Эта проблема описана в статье «Гугл-программисты. Как идиот набрал на работу идиотов».
Предупреждаю, это провокационная статья, которая собрала много критики. Однако, мне кажется, многие читатели зря цепляются к словам и грубому стилю повествования. Эта статья — гротеск! В ней проблема специально подаётся жёстко. А без этого, возможно, статья и не получила бы такого внимания аудитории и осталась незамеченной. В общем, почитайте, в ней поднята очень интересная и важная проблема современного мира программирования.
Дополнительно можно познакомиться с комментариями к статье:
- Hacker News;
- Reddit;
- Slashdot;
- daily.dev (здесь вообще эпично: «жёлтый журналист» придумал вбросить, что речь идёт про разработчиков PVS-Studio:-/).
На всякий случай, ещё раз: это сторонняя статья. Она написана не кем-то из нашей команды. Нас заинтриговал текст, и мы запросили разрешение у автора перевести её и разместить в своём блоге.
Кстати, я на собеседовании спрашиваю, какие книги по программированию на C++ читал соискатель. Это не значит, что мы не возьмём человека, если он ответит, что не читал книг, но это повод насторожиться.
Ещё мы просим написать на бумаге пару небольших функций, например «посчитать количество нулевых бит в переменной типа unsigned int». Это сразу очень много говорит о кандидате. Впрочем, нас больше интересуют всесторонние теоретические знания языка C++ в силу специфики нашей работы по созданию анализатора кода.
P.S.: Может показаться, что я против того, чтобы что-то поискать в интернете. Вовсе нет. Googling is one of the most important skills for every developer. Просто умения поиска не могут заменить собственные знания и умения писать сложный код.
Вредный совет N58. printf (str);
Нужно распечатать строку? Что тут думать! Берёшь и печатаешь её с помощью printf!
Когда нужно распечатать или, например, записать строку в файл, многие программисты, не задумываясь, пишут код следующего вида:
printf(str);
fprintf(file, str);Это очень опасные конструкции. Дело в том, что если внутрь строки каким-то образом попадёт спецификатор форматирования, то это приведёт к непредсказуемым последствиям.
Допустим, мы хотим использовать следующий код для распечатки имени файла:
printf(name().c_str());Если у файла будет имя «file%s%i%s.txt», то программа может упасть или распечатать белиберду.
Но это только полбеды. Фактически такой вызов функции — это самая настоящая потенциальная уязвимость, с её помощью можно пытаться атаковать программы. Подготовив особенным образом строки, можно распечатать приватные данные, хранимые в памяти.
Подробнее про подобные уязвимости можно прочитать в этой статье. Найдите время с ней познакомиться, думаю, будет интересно. В ней не только рассматривается теория, но и приводятся практические примеры.
Корректный код:
printf("%s", name().c_str());Вообще, такие функции, как printf, sprintf, fprintf, опасны. Лучше их не использовать, а воспользоваться чем-то более современным. Например, вам могут пригодиться boost: format, std: stringstream или std: format.
Ещё можно вот это видео посмотреть.
Вредный совет N59. Виртуальные функции в конструкторах и деструкторах
Стандарт C++ чётко определяет, как работает вызов виртуальной функции в конструкторе и деструкторе. Значит, смело можно делать такие вызовы.
Абстрактно — это действительно так. Вот только на практике неправильное использование виртуальных функций — это классическая ошибка при разработке на языке С++. Причин две:
- Можно пользоваться виртуальными функциями и не знать об особенностях их работы в конструкторах/деструкторах. Или забыть. Это подтверждается обсуждениями, например на сайте Stack Overflow.
- В разных языках программирования поведение виртуальных функций отличается. Это добавляет путаницы.
Поэтому я подробно разобрал эти две причины и ошибки в статье «Вызов виртуальных функций в конструкторах и деструкторах (C++)».
Рекомендую к прочтению эту статью всем, кто сейчас изучает программирование на языке C++. Впрочем, даже бывалым программистам может оказаться полезным освежить познания в этой области. Особенно тем, кто пишет код сразу на нескольких языках.
Вредный совет N60. Нет времени думать, копируй код!
Копирование кода с последующей правкой — самый быстрый способ написания кода. Да здравствует Copy-Paste программирование!
Действительно, так можно быстро написать много кода. Особенно это удобно, если ваша работа оценивается по количеству строк кода :).
Однако если мы говорим о разработке качественного приложения, а не о том, как создать больше «говнокода», то у такого подхода есть три больших недостатка:
- Избыточный код. Кода больше, чем необходимо, для решения задачи. А чем больше кода, тем больше сил тратится на его обзоры, сопровождение, правку, отладку. Например, программист нашёл и исправил ошибку в коде, но забыл, что такой же фрагмент кода он использовал в другой функции. В результате вторая ошибка останется в коде и, когда она проявит себя, программисту придётся повторно её искать.
- Необходимость множественных правок. При использовании методики Copy-Paste программа будет содержать схожие блоки кода, выполняющие приблизительно одни и те же действия. Когда нужно изменить логику поведения программы, высока вероятность, что придётся модифицировать все эти похожие места. Это отнимает время, и есть вероятность где-то забыть внести правку.
- Ошибки. Копируя и затем модифицируя код, очень легко забыть исправить какое-то имя или числовой литерал. Возникают ошибки, которые сложно заметить на обзоре кода, так как блоки текста похожи и тяжело их проверять, не теряя внимание. В блоге PVS-Studio мы регулярно описываем подобные ошибки, которые находим в открытых проектах.
Исправить ситуацию можно, не копируя код, а вынося его в функции и затем используя из различных частей программы. Это устраняет все три проблемы:
- Нет избыточного кода;
- Логика работы правится только в одном месте;
- Уменьшается вероятность допустить ошибку.
Рассмотрим, как это может выглядеть на практике. Показанная ниже ошибка была найдена нами в проекте Blend2D.
static BLResult BL_CDECL bl_convert_argb32_from_prgb_any(
const BLPixelConverterCore* self,
uint8_t* dstData, intptr_t dstStride,
const uint8_t* srcData, intptr_t srcStride,
uint32_t w, uint32_t h, const BLPixelConverterOptions* options) noexcept {
if (!options)
options = &blPixelConverterDefaultOptions;
const size_t gap = options->gap;
dstStride -= uintptr_t(w) * 4 + gap;
srcStride -= uintptr_t(w) * PixelAccess::kSize;
const BLPixelConverterData::NativeFromForeign& d =
blPixelConverterGetData(self)->nativeFromForeign;
uint32_t rMask = d.masks[0];
uint32_t gMask = d.masks[1];
uint32_t bMask = d.masks[2];
uint32_t aMask = d.masks[3];
uint32_t rShift = d.shifts[0];
uint32_t gShift = d.shifts[1];
uint32_t bShift = d.shifts[2];
uint32_t aShift = d.shifts[3];
uint32_t rScale = d.scale[0];
uint32_t gScale = d.scale[1];
uint32_t bScale = d.scale[2];
uint32_t aScale = d.scale[3];
for (uint32_t y = h; y != 0; y--) {
if (!AlwaysUnaligned && blIsAligned(srcData, PixelAccess::kSize)) {
for (uint32_t i = w; i != 0; i--) {
uint32_t pix = PixelAccess::fetchA(srcData);
uint32_t r = (((pix >> rShift) & rMask) * rScale) >> 16;
uint32_t g = (((pix >> gShift) & gMask) * gScale) >> 8;
uint32_t b = (((pix >> bShift) & bMask) * bScale) >> 8;
uint32_t a = (((pix >> aShift) & aMask) * aScale) >> 24;
BLPixelOps::unpremultiply_rgb_8bit(r, g, b, a);
blMemWriteU32a(dstData, (a << 24) | (r << 16) | (g << 8) | b);
dstData += 4;
srcData += PixelAccess::kSize;
}
}
else {
for (uint32_t i = w; i != 0; i--) {
uint32_t pix = PixelAccess::fetchA(srcData);
uint32_t r = (((pix >> rShift) & rMask) * rScale) >> 16;
uint32_t g = (((pix >> gShift) & gMask) * gScale) >> 8;
uint32_t b = (((pix >> bShift) & bMask) * bScale) >> 8;
uint32_t a = (((pix >> aShift) & aMask) * aScale) >> 24;
BLPixelOps::unpremultiply_rgb_8bit(r, g, b, a);
blMemWriteU32a(dstData, (a << 24) | (r << 16) | (g << 8) | b);
dstData += 4;
srcData += PixelAccess::kSize;
}
}
dstData = blPixelConverterFillGap(dstData, gap);
dstData += dstStride;
srcData += srcStride;
}
return BL_SUCCESS;
}В данном фрагменте кода then и else-ветки полностью совпадают. Этот код явно писался методом Copy-Paste. Программист скопировал внутренний цикл, а затем отвлёкся и забыл заменить во втором цикле вызов функции PixelAccess: fetchA на PixelAccess: fetchU.
К счастью, статический анализатор PVS-Studio заметил аномалию и выдал предупреждение: V523 The 'then' statement is equivalent to the 'else' statement. pixelconverter.cpp 1215
Эта классическая Copy-Paste ошибка появилась из-за того, что программист немного поленился при написании кода. Есть как минимум 3 способа, как избежать дублирования кода и заодно снизить вероятность рассмотренной ошибки.
Первый вариант — можно вынести внутренний цикл в отдельную функцию и передавать ей дополнительный флажок, который будет указывать, следует вызывать PixelAccess: fetchA или PixelAccess: fetchU.
Этот способ мне не нравится, так как этой новой функции придётся передавать очень много аргументов. Поэтому не будем его подробнее рассматривать. Отмечу только, что даже такой вариант снижает вероятность ошибки. Сложно забыть использовать аргумент функции (флажок). Тем более даже компилятор предупредит, что аргумент не используется.
Второй вариант — вынести общий код в лямбда функцию. Тогда код может выглядеть приблизительно так:
static BLResult BL_CDECL bl_convert_argb32_from_prgb_any(
const BLPixelConverterCore* self,
uint8_t* dstData, intptr_t dstStride,
const uint8_t* srcData, intptr_t srcStride,
uint32_t w, uint32_t h, const BLPixelConverterOptions* options) noexcept {
if (!options)
options = &blPixelConverterDefaultOptions;
const size_t gap = options->gap;
dstStride -= uintptr_t(w) * 4 + gap;
srcStride -= uintptr_t(w) * PixelAccess::kSize;
const BLPixelConverterData::NativeFromForeign& d =
blPixelConverterGetData(self)->nativeFromForeign;
uint32_t rMask = d.masks[0];
uint32_t gMask = d.masks[1];
uint32_t bMask = d.masks[2];
uint32_t aMask = d.masks[3];
uint32_t rShift = d.shifts[0];
uint32_t gShift = d.shifts[1];
uint32_t bShift = d.shifts[2];
uint32_t aShift = d.shifts[3];
uint32_t rScale = d.scale[0];
uint32_t gScale = d.scale[1];
uint32_t bScale = d.scale[2];
uint32_t aScale = d.scale[3];
auto LoopImpl = [&](const bool a)
{
for (uint32_t i = w; i != 0; i--) {
uint32_t pix = a ? PixelAccess::fetchA(srcData) :
PixelAccess::fetchU(srcData);
uint32_t r = (((pix >> rShift) & rMask) * rScale) >> 16;
uint32_t g = (((pix >> gShift) & gMask) * gScale) >> 8;
uint32_t b = (((pix >> bShift) & bMask) * bScale) >> 8;
uint32_t a = (((pix >> aShift) & aMask) * aScale) >> 24;
BLPixelOps::unpremultiply_rgb_8bit(r, g, b, a);
blMemWriteU32a(dstData, (a << 24) | (r << 16) | (g << 8) | b);
dstData += 4;
srcData += PixelAccess::kSize;
}
}
for (uint32_t y = h; y != 0; y--) {
LoopImpl(!AlwaysUnaligned && blIsAligned(srcData, PixelAccess::kSize));
dstData = blPixelConverterFillGap(dstData, gap);
dstData += dstStride;
srcData += srcStride;
}
return BL_SUCCESS;
}На самом деле, для рассматриваемого примера выносить что-то в функцию вообще нет необходимости. Но мне хотелось показать, как подход с функцией может сокращать код и уменьшать вероятность ошибки.
Третий вариант — самый простой и естественный — делать проверку внутри цикла.
static BLResult BL_CDECL bl_convert_argb32_from_prgb_any(
const BLPixelConverterCore* self,
uint8_t* dstData, intptr_t dstStride,
const uint8_t* srcData, intptr_t srcStride,
uint32_t w, uint32_t h, const BLPixelConverterOptions* options) noexcept {
if (!options)
options = &blPixelConverterDefaultOptions;
const size_t gap = options->gap;
dstStride -= uintptr_t(w) * 4 + gap;
srcStride -= uintptr_t(w) * PixelAccess::kSize;
const BLPixelConverterData::NativeFromForeign& d =
blPixelConverterGetData(self)->nativeFromForeign;
uint32_t rMask = d.masks[0];
uint32_t gMask = d.masks[1];
uint32_t bMask = d.masks[2];
uint32_t aMask = d.masks[3];
uint32_t rShift = d.shifts[0];
uint32_t gShift = d.shifts[1];
uint32_t bShift = d.shifts[2];
uint32_t aShift = d.shifts[3];
uint32_t rScale = d.scale[0];
uint32_t gScale = d.scale[1];
uint32_t bScale = d.scale[2];
uint32_t aScale = d.scale[3];
for (uint32_t y = h; y != 0; y--) {
const bool a = !AlwaysUnaligned && blIsAligned(srcData, PixelAccess::kSize);
for (uint32_t i = w; i != 0; i--) {
uint32_t pix = a ? PixelAccess::fetchA(srcData) :
PixelAccess::fetchU(srcData);
uint32_t r = (((pix >> rShift) & rMask) * rScale) >> 16;
uint32_t g = (((pix >> gShift) & gMask) * gScale) >> 8;
uint32_t b = (((pix >> bShift) & bMask) * bScale) >> 8;
uint32_t a = (((pix >> aShift) & aMask) * aScale) >> 24;
BLPixelOps::unpremultiply_rgb_8bit(r, g, b, a);
blMemWriteU32a(dstData, (a << 24) | (r << 16) | (g << 8) | b);
dstData += 4;
srcData += PixelAccess::kSize;
}
dstData = blPixelConverterFillGap(dstData, gap);
dstData += dstStride;
srcData += srcStride;
}
return BL_SUCCESS;
}Мы рассмотрели красивый пример, где можно избавиться от дублирования кода. Однако так бывает не всегда. Иногда явно виден Copy-Paste и вызванная этим ошибка, но непонятно, как можно было этого избежать. Рассмотрим такой случай на примере бага, найденного мною в LLVM 15.0:
static std::unordered_multimap
loadDataFiles(const std::string &NamesFile, const std::string &AliasesFile) {
....
auto FirstSemiPos = Line.find(';');
if (FirstSemiPos == std::string::npos) // <=
continue;
auto SecondSemiPos = Line.find(';', FirstSemiPos + 1);
if (FirstSemiPos == std::string::npos) // <=
continue;
....
} Код писался с помощью Copy-Paste. Был размножен этот фрагмент:
auto FirstSemiPos = Line.find(';');
if (FirstSemiPos == std::string::npos)
continue;Затем поменяли вызов функции find. First заменили на Second, но не везде. В результате строчка
if (FirstSemiPos == std::string::npos)осталась без изменений. Так как это условие уже проверялось выше, анализатор PVS-Studio сообщает, что условие всегда ложно: V547 Expression 'FirstSemiPos == std: string: npos' is always false. UnicodeNameMappingGenerator.cpp 46
В данном случае, нечего обобщать и выносить в функцию. И непонятно, как по-другому написать этот код, чтобы избежать ошибки.
Кто-то может сказать: надо было не копировать код, а написать его заново. Тогда бы и ошибки не было. А если хочется что-то скопировать — это повод остановиться, подумать и сделать функцию или по-другому написать код.
Звучит разумно, но я не верю в такой подход. Что бы там ни говорили, но иногда быстро, удобно и эффективно именно скопировать фрагмент кода. В общем, я не буду столь радикален, чтобы предлагать никогда не использовать Copy-Paste.
Более того, я вообще сомневаюсь в искренности людей, которые призывают никогда не использовать копирование кода. Небось, оставили умный комментарий, чтобы казаться идеальней других, и пошли дальше Copy-Patse в своём проекте делать :).
В общем, все копировали код и будут продолжать это делать. Да, количество таких случаев можно и нужно сокращать. Однако надо признать, что всё равно какие-то фрагменты будут копироваться, и это будет приводить к ошибкам. Что же делать?
Советы всё те же:
- писать простой понятный код. В нём будет легче заметить ошибку;
- использовать форматирование кода таблицей (см. совет N20);
- использовать статические анализаторы кода.
Вредный совет N61. Можно заглянуть за пределы массива
Раз допустимо ссылаться на следующий элемент за пределами массива, то, значит, можно к этому элементу и обращаться.
Ой, это же уже 61-й пункт списка, а их должно было быть 60. Сорри, но в коллекции вредных советов обязан быть выход за границу массива!
Выход за границу массива приводит к неопределённому поведению. Однако есть один момент, который может смутить неопытного программиста.
В C++ допустимо ссылаться на элемент, лежащий за последним элементом массива. Например, допустим следующий код:
int array[5] = { 0, 1, 2, 3, 4 };
int *P = array + 5;Однако значение указателя P можно только сравнивать с другими значениями, но не разыменовывать.
Подобное допущение позволяет построить изящную концепцию итераторов. В классах для работы с массивами функция end возвращает итератор, указывающий на условный элемент, находящийся за последним элементом контейнера. Итератор end можно сравнивать с другими итераторами, но нельзя разыменовывать.
Помимо этого, программисты просто по невнимательности допускают ошибку, выходя на 1 элемент за пределы массива. У такой ошибки даже есть название — off-by-one error. Причина в том, что элементы в массиве нумеруются с 0, и это иногда сбивает с толку, особенно при поспешном написании кода.
Чаще всего ошибка возникает из-за неправильной проверки индекса. Проверяют, что индекс не больше количества элементов в массиве. Но это неправильно, так как если индекс равен количеству элементов, то он уже ссылается на элемент за пределами массива. Поясним это практическим примером.
Следующая ошибка была найдена статическим анализатором PVS-Studio в Clang 11. Так что, как видите, подобные ляпы допускают не только новички.
std::vector DeclsLoaded;
SourceLocation ASTReader::getSourceLocationForDeclID(GlobalDeclID ID) {
....
unsigned Index = ID - NUM_PREDEF_DECL_IDS;
if (Index > DeclsLoaded.size()) {
Error("declaration ID out-of-range for AST file");
return SourceLocation();
}
if (Decl *D = DeclsLoaded[Index])
return D->getLocation();
....
} Правильная проверка должна быть такой:
if (Index >= DeclsLoaded.size()) {Конец
Спасибо за внимание и потраченное время. Буду признателен, если вы поделитесь ссылками со своими коллегами. И напишите, с чем вы не согласны или, наоборот, где я точно попал в больное место :).
Об этой мини-книге
Автор: Карпов Андрей Николаевич. E-Mail: karpov [@] viva64.com.
Более 15 лет занимается темой статического анализа кода и качества программного обеспечения. Автор большого количества статей, посвящённых написанию качественного кода на языке C++. С 2011 по 2021 год удостаивался награды Microsoft MVP в номинации Developer Technologies. Один из основателей проекта PVS-Studio. Долгое время являлся CTO компании и занимался разработкой С++ ядра анализатора. Основная деятельность на данный момент — управление командами, обучение сотрудников и DevRel активность.
Ссылки на полный текст:
Подписывайтесь на ежемесячную рассылку, чтобы не пропустить другие публикации автора и его коллег.
