О вечном — о разумной длине строк кода. Мы недавно встретили ошибку, которая одновременно демонстрирует, чем плох "код-колбаса", "эффект последний строки" и последствия неудачного copy-paste.
Количество диагностик в анализаторе PVS-Studio перевалило за 1100, и с каждым релизом продолжает расти. В следующем году вообще предсказуем их количественный скачок, связанный с поддержкой новых языков. Но про это пока говорить рано.
Однако мы не только реализуем новые диагностические правила (детекторы), но и постепенно дорабатываем старые. Например, в декабрьский релиз войдёт улучшение в C# одой из самых старых диагностик — V3001. Мы научили её корректнее определять ситуации, когда дополнительные круглые скобки ни на что не влияют. Результат — выявление новых ошибок, с одной из которых мы сейчас познакомимся.
Баг найден в проекте Space Engineers, который входит в базу проектов для регрессионного тестирования нашего анализатора. Если интересны подробности, то недавно мы касались нашей системы регрессионного тестирования в вебинаре, посвящённом функциональному тестированию.
Мы используем старую зафиксированную версию проекта, так как нам требуется анализировать отличие работы PVS-Studio на одной и той же кодовой базе. Впрочем, если заглянуть в свежую версию кода, то можно обнаружить, что ошибка по-прежнему присутствует. См. функцию Contains в BoundingBox.cs.
Видите суслика баг? Думаю, нет. А он есть!
А всё почему? Потому что вредно писать такие длинные нечитаемые строки кода. В них очень легко просмотреть ошибку, что, собственно, мы и наблюдаем. Чтобы всё-таки иметь возможность понять, в чём дело, давайте хоть немного отформатируем код.
public ContainmentType Contains(BoundingSphere sphere)
{
Vector3 result1;
Vector3.Clamp(ref sphere.Center, ref this.Min,
ref this.Max, out result1);
float result2;
Vector3.DistanceSquared(ref sphere.Center, ref result1, out result2);
float num = sphere.Radius;
if ((double)result2 > (double)num * (double)num)
return ContainmentType.Disjoint;
return (double)this.Min.X + (double)num > (double)sphere.Center.X ||
(double)sphere.Center.X > (double)this.Max.X - (double)num ||
((double)this.Max.X - (double)this.Min.X <= (double)num ||
(double)this.Min.Y + (double)num > (double)sphere.Center.Y) ||
((double)sphere.Center.Y > (double)this.Max.Y - (double)num ||
(double)this.Max.Y - (double)this.Min.Y <= (double)num ||
((double)this.Min.Z + (double)num > (double)sphere.Center.Z ||
(double)sphere.Center.Z > (double)this.Max.Z - (double)num)) ||
(double)this.Max.X - (double)this.Min.X <= (double)num ?
ContainmentType.Intersects : ContainmentType.Contains;
}
Так лучше, но всё равно надо сосредоточиться, чтобы заметить ошибку. Взгляните на последнюю строчку в логическом условии:
(double)this.Max.X - (double)this.Min.X <= (double)num
Это дубликат третьей строчки. Верхнее условие заключено в дополнительные скобки, но они никак не влияют на результат вычислений, так как все проверки объединяются с помощью ИЛИ (оператор ||).
На самом деле, задумывалось, что в последней строке должна проверяться координата Z:
(double)this.Max.Z - (double)this.Min.Z <= (double)num
PVS-Studio теперь замечает эту аномалию и выдаёт предупреждение:
V3001 There are identical sub-expressions '(double)this.Max.X - (double)this.Min.X <= (double)num' to the left and to the right of the '||' operator.
Хорошая демонстрация, как статический анализатор дополняет обзор кода, в процессе которого сложно заметить опечатку в такой длинной строке. Я называю это "кодом-колбасой" и уже писал заметку о том, как такая "колбаса" притягивает баги. Хочется привести здесь картинку из той публикации.
Как видите, эта ситуация относится и к C# программистам :)
В рассмотренном коде проявился и "эффект последней строки". Опечатки часто появляются в конце однотипных фрагментов (блоков) кода. Здесь, правда, нельзя говорить именно про строки, так как строка одна. Но, по сути, проблема всё та же: баг находится в последней части длинного выражения, состоящего из схожих последовательностей лексем.
Ошибка возникла из-за написания кода copy-paste подходом: подвыражения, видимо, писались с помощью копирования предыдущих. В одном месте в копию не были внесены требуемые изменения касательно координаты Z.
Но это ещё не всё! Вся эта прекрасная длинная строка с ошибкой ещё раз была размножена копированием. Её можно наблюдать в соседней функции Contains несколькими строками ниже:
public void Contains(ref BoundingSphere sphere, out ContainmentType result)
{
....
if ((double)result2 > (double)num * (double)num)
result = ContainmentType.Disjoint;
else
result = (double)this.Min.X + (double)num > (double)sphere.Center.X ||
(double)sphere.Center.X > (double)this.Max.X - (double)num ||
((double)this.Max.X - (double)this.Min.X <= (double)num ||
(double)this.Min.Y + (double)num > (double)sphere.Center.Y) ||
((double)sphere.Center.Y > (double)this.Max.Y - (double)num ||
(double)this.Max.Y - (double)this.Min.Y <= (double)num ||
((double)this.Min.Z + (double)num > (double)sphere.Center.Z ||
(double)sphere.Center.Z > (double)this.Max.Z - (double)num)) ||
(double)this.Max.X - (double)this.Min.X <= (double)num ?
ContainmentType.Intersects : ContainmentType.Contains;
}
Всё то же самое, и анализатор указал на вторую ошибку таким же предупреждением.
Заключение
Пропущу развёрнутое наставление о том, почему такой код плох и как его следует изменить, чтобы избежать подробных ошибок. Уверен, постоянные читатели нашего блога уже догадываются, что всё сводится к:
Разработке в компании регламента (стандарта написания кода), где будет запрет на слишком длинные строки кода. Можно посмотреть, как это написано у нас.
Регулярному использованию статического анализатора PVS-Studio для дополнительной проверки кода.
Удалению лишних операций. Например, чтобы не выполнять везде приведение типа
(double)num, можно было сразу объявить переменнуюnumкакdouble.Вынесению единообразного кода в функции.
Комментарии (39)
panzerfaust
01.12.2025 09:01Чинить баг в чистой функции SASTом это воистину забивать гвозди микроскопом. Этот код без юнит-тестов не имеет права существовать, как его не форматируй.
Alex283
01.12.2025 09:01Как говорил нам преподаватель, что хорошо читаемый код, это не тот который красиво выравнен табуляциями и пробелами, а тот который оформлен осмысленно.
"Мелкие" не знают, а помню, когда это дичь началась! Разве еще остались те, кто просматривает код на VGA мониторах?
VBDUnit
01.12.2025 09:01Мне кажется смысл тут в том, чтобы код был написан так, чтобы глаза цеплялись за важное и не цеплялись за неважное. Чем больше расхождение, тем выше вероятность что‑то пропустить, поэтому нужно учитывать особенности зрения.
Глаза эволюционно сделаны для поиска всяких бананов в листве и мамонтов с саблезубыми кошками в траве. Картинка обрабатывается уже на уровне сетчатки, выделяются ключевые паттерны и передаются в зрительный нерв, вроде бы даже доказано, что там какое‑то подобие разложения Фурье происходит.
Если глаз видит кучу подобных объектов (типа травинок), то они передаются в мозг не по отдельности, а как «куча объектов которые в среднем похожи на травинку» или вообще как шум такого‑то цвета такой‑то интенсивности.
ИМХО проблема кода типа
result = (double)this.Min.X + (double)num > (double)sphere.Center.X || (double)sphere.Center.X > (double)this.Max.X - (double)num || ((double)this.Max.X - (double)this.Min.X <= (double)num || (double)this.Min.Y + (double)num > (double)sphere.Center.Y) || ((double)sphere.Center.Y > (double)this.Max.Y - (double)num || (double)this.Max.Y - (double)this.Min.Y <= (double)num || ((double)this.Min.Z + (double)num > (double)sphere.Center.Z || (double)sphere.Center.Z > (double)this.Max.Z - (double)num)) || (double)this.Max.X - (double)this.Min.X <= (double)num ? ContainmentType.Intersects : ContainmentType.Contains;не в длине строки как таковой, а в том, что тут куча подобных объектов, слабо отличающихся друг от друга. И зрение «на аппаратном уровне» это не фильтрует. Это буквально аналог всяких загадок типа такой.
Найдите среди букв Q букву O без хвостика Тут, ИМХО, можно не просто ловить какие‑то конкретные паттерны типа длинных строк или множества однотипных конструкций, которые уязвимы перед зрением, а генерировать для кода тепловую карту удобочитаемости (сеткой и/или алгоритмами). Карту можно выводить прямо в IDE (типа под плохим кодом постепенно проступают красные пятна) или просто искать на карте экстремумы и выводить их в лог.
datacompboy
01.12.2025 09:01Hidden text
5я строка 2й столбец...
VBDUnit
01.12.2025 09:01Скрытый текст
Да) Я сам минут 10 искал, и таки нашёл. Сейчас решил эксперимент поставить на локальной Qwen3-vl-30b
Она почти сразу распрощалась с кукухой
Давайте внимательно посмотрим на изображение. Все буквы выглядят как Q со странным шрифтом и хвостиком, но, как и сказано, одна из них — это буква O. Разберёмся: Буква Q обычно имеет хвостик, который выходит вправо-вниз. Буква O — это просто круг, без хвостика. Значит, нам нужно найти ту единственную букву, которая не имеет хвостика — то есть, это O. Теперь посмотрим на изображение. Всё выглядит одинаково, но если внимательно рассмотреть, можно заметить, что все буквы — это Q, но в одной из них хвостик отсутствует. Посмотрим на вторую строку снизу, второй столбец слева. Нет, давайте пересчитаем. Посмотрим на вторую строку снизу, второй столбец слева — там, кажется, хвостик отсутствует. Но давайте уточним. На самом деле, если внимательно посмотреть, то все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте проверим. На самом деле, все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвостика. Но давайте уточним: все буквы — это Q, но в одной из них хвостик отсутствует — это и есть O. Посмотрим на вторую строку снизу, второй столбец слева — да, там нет хвоqyix7z
01.12.2025 09:01Я проверил на Perplexity, гигачате, Qwen3-Max - никто не дал правильного ответа. Дипсик вообще не нашел текста.
Zalechi
01.12.2025 09:01А я сразу увидел нестыковку, и только потом узнал задание и тут же его закрыл, ибо нашел уже)))
Повезло
aamonster
01.12.2025 09:01Плохой пример, она тут в глаза бросается, как тёмное пятно на общем фоне. Во всяком случае, если смотришь на телефоне (буквы мелкие), а зрение не идеальное: чуть расфокусируешь взгляд – и видишь жирную точку на чёрной горизонтальной линии.
datacompboy
01.12.2025 09:01Расслабить глаза и размылить картинку - эти стандартный хак для быстрого решения.
Так же как "раскосить" (или "свести") а-ля 3Д картинки что быстро найти N отличий.aamonster
01.12.2025 09:01Угу, поэтому обычно подобные картинки защищены от такого примитивного метода :-)
ThunderFromTheSky
01.12.2025 09:01Ну нафиг такие извращения! Если разраб пишет код, который возможно прочитать после таких манипуляций - выгнать немедленно такого говнокодера.
Zalechi
01.12.2025 09:01Может и не плохой пример, но мне да, тоже сразу в глаза бросилось, а другие пишут, что искали десят минут, ¯\_(ツ)_/¯
muhachev
01.12.2025 09:01Только код это не бананы в траве у саблезубых. Вторая сигнальная больше ориентирована на смысл слов, а вцепляться в глаз - это скорее из арсенала маркетологов. Степень важности определяется всегда сугубо субъективно и индивидуально. И в программном коде, если конечно его писал профи, а не ascii-маляр, с точки зрения смысла важно должно быть всё, ибо если что-то неважно, то его не должно и быть.
buldo
01.12.2025 09:01Это хорошо, что теперь и на такое будут предупреждения.
Однако, IMHO, проблема вообще в самом этом страшном выражении.
Такое ощущение, что его сначала написали более-менее нормально, а потом другой анализатор сказал "ой, а что это такое, давай сделаем тернарный if". И в итоге всё превратилось в нечитаемую строку.
Akon32
01.12.2025 09:01В примере какой-то дизассемблер, слишком много кастов. Если код почистить от них, он сразу будет лучше читаться.
wert_lex
01.12.2025 09:01Когда-то очень давно в университете у нас были пары по численным методам. Вёл их очень харизматичный и вредный дед. К тому моменту я уже неплохо программировал, но сдать ему лабы было совершенно невозможно. Он носил очень толстые очки, всматривался в упор в пузатый 15 дюймовый ЭЛТ монитор, тыкал в него пальцем и говорил "неправильно" слегка сдабривая сказанное брызгами слюны.
Примерно в этом время мы и открыли для себя методы заэкранного программирования. Очень удобная штука: на экране какой-нибудь метод Рунге-Кутты 4-го порядка, а за экраном - правильный ответ, который и выводился на экран.
С тех пор длинные строки и не люблю
SecondUniverse
01.12.2025 09:01Вы книги тоже больше одного абзаца прочесть не можете и смысл теряете при перелистывании?
ImagineTables
01.12.2025 09:01Для программирования на Java монитор должен быть не только горизонтальный, но и вертикальный, иначе при отладке колстек не влезет.
VBDUnit
01.12.2025 09:01монитор должен быть не только горизонтальный, но и вертикальный
..или просто большой.
itstranger
01.12.2025 09:01Когда много условий, как в примере, иногда выношу их в отдельные массивы. Переписал код из примера на C#, если что не верно, не ругайте (писал без компилятора под рукой, только попросил нейронку синтаксис поправить), но суть думаю понятна.
Вместо того, чтобы все условия писать в строчки, я их группирую в массивы, по смыслу.
После, чего использую any() или другие похожие функции в других языках, чтобы реализовать "или" (если нужно "и" можно использовать all() или ему подобные, либо вообще написать эти функции самому они не то чтобы сложные цикл + условие). Если в исходном условии есть скобки, можно один массив вкладывать в другой.
Плюсы такого подхода? Как по мне, более читаемо, по массивам сразу видно, что именно проверяется, чем когда все условия в куче. Удобнее тестировать, в дебаге видны массивы результатов всех условий. Лично мне, проще вносить изменения в подобные массивы, т.к. точно не нарушу скобки. Если говорить по теме, то с таким подходом можно любые длинные условия развернуть и сгруппировать по смыслу.
Из минусов: any() и подобные функции генерируют больше операций (т.к. под капотом там цикл и условия), однако если мы работаем с фиксированным небольшим набором данных, то нагрузка хоть и увеличивается, но разница нулевая. Опять же, всё зависит от конкретного случая, поэтому и не использую такой подход повсеместно.
VBDUnit
01.12.2025 09:01Если я правильно понимаю, то проверки для всех трёх измерений идентичны, поэтому можно сделать как‑то так:
public ContainmentType Contains(BoundingSphere sphere) { float3 clampedCenter = float3.Clamp(sphere.Center, Min, Max); float radius = sphere.Radius; { var distanceSquared = float3.DistancePow2(sphere.Center, clampedCenter); if (distanceSquared > radius * radius) return ContainmentType.Disjoint; } float3 thisSize = Max - Min; for (int dimensionIndex = 0; dimensionIndex < 3; dimensionIndex++) { //if (dimensionIndex is 0 or 1) if (thisSize[dimensionIndex] <= radius) return ContainmentType.Intersects; if (Min[dimensionIndex] > sphere.Center[dimensionIndex] - radius) return ContainmentType.Intersects; if (sphere.Center[dimensionIndex] > Max[dimensionIndex] - radius) return ContainmentType.Intersects; } return ContainmentType.Contains; }Массивы в C# — это аллокации в куче. В подавляющем большинстве случаев на них пофиг, особенно если они короткоживущие. Но если среда выполнения обладает кривым сборщиком мусора и код выполняется слишком часто, это может вызывать подлагивания даже на 0 поколении.
В.NET 10, ЕМНИП, завезли аллокацию массивов на стеке, если компилятор видит, что это не разрушит логику, ИМХО, Ваш пример туда как раз попадает. Поэтому в.NET 10 проблем точно не должно быть.
itstranger
01.12.2025 09:01А, точно в C# же куча постоянно живёт и GC глохнет если очень много аллокаций. Эхх, понемногу начинаю, забывать специфику C#.( Надо бы на нём, что-то написать, чтобы обновить в памяти.
Да, вы правы мой код в C#, при очень частых вызовах будет "грузить" GC. Я кстати поэтому упомянул, что помещение условий в массив, не всегда уместна.)
Кстати мой пример, чтобы не грузить GC аллокациями можно, переделав, переместив условия из массивов в обычные методы/функции.
Кстати вы верно подметили, и написали хороший вариант для рефакторинга оригинального примера из статьи, там в целом код, как понял проблемный.
StreamThread
Dotnet'у, да и другим языкам, несмотря на синтаксис очень полезно было бы иметь рекомендованный кодингстайл как у Python
Andrey2008 Автор
Кто мешает в команде использовать свой стандарт кодирования?
Akon32
...и каждый раз случаются долгие споры, как "правильно" писать буквы.
ThunderFromTheSky
Для того и принимается стандарт тимлидом команды
Goron_Dekar
А лучше несколько, как у C++.
С конкуренцией. Потому что конкуренция - отличный источник прогресса.
VBDUnit
Добросовестная конкуренция. Иначе это источник не прогресса, а кое‑чего другого.
Goron_Dekar
Даже без этого дополнения оно лучше, чем монополия. Даже если монополия комитета по стандартизации.
pavel_shabalin
Спасибо конечно, но пожалуй не стоит...