Старайтесь не писать строки кода, которые не помещаются на экран

24 Ноя 2025

Автор: Андрей Карпов

Встретили баг, который одновременно демонстрирует, как "код-колбаса" притягивает ошибку, эффект последней строки и проблему неудачного copy-paste.

Мы не только реализуем в PVS-Studio новые диагностические правила, но и постепенно дорабатываем старые. Например, недавно в C#-анализаторе мы улучшили одну из самых старых диагностик — V3001, научив её корректнее определять случаи, когда дополнительные скобки ни на что не влияют. Результат — выявление новых багов, с одним из которых мы сейчас вас познакомим.

Ошибка найдена в проекте Space Engineers, который входит в базу проектов для регрессионного тестирования нашего анализатора. Мы используем старую зафиксированную версию проекта, так как нам нужно анализировать отличие работы анализатора на одном и том же коде. Но если заглянуть в свежую версию исходного кода, то можно обнаружить, что ошибка по-прежнему присутствует. См. функцию 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.

Хороший пример, когда статический анализатор дополняет обзор кода, в процессе которого сложно заметить опечатку в такой длинной строке. Я называю это "кодом-колбасой" и уже писал заметку о том, как он притягивает баги.

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

Ошибка возникла из-за copy-paste. Подвыражения, видимо, писались с помощью копирования предыдущих, и в одном из мест в копию не были внесены нужные изменения. Однако это ещё не всё. Вся эта строка с ошибкой ещё раз была размножена копированием, и её можно наблюдать в соседней функции 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;
}

Всё то же самое, и анализатор указал на вторую ошибку точно таким же предупреждением.

Заключение

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

Форматированию кода таблицей.
Вынесению единообразного кода в функции.
Удалению лишних операций. Например, вместо того, чтобы везде выполнять приведение типа (double)num можно было сразу объявить переменную num как double.
Регулярному использованию статического анализатора PVS-Studio для дополнительного контроля.

#CSharp