О вечном — о разумной длине строк кода. Мы недавно встретили ошибку, которая одновременно демонстрирует, чем плох "код-колбаса", "эффект последний строки" и последствия неудачного 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;
}

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

Заключение

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

1. Разработке в компании регламента (стандарта написания кода), где будет запрет на слишком длинные строки кода. Можно посмотреть, как это написано у нас.

2. Регулярному использованию статического анализатора PVS-Studio для дополнительной проверки кода.

3. Форматированию кода таблицей.

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

5. Вынесению единообразного кода в функции.