Грязный код — надёжное хранилище ошибок. Теория разбитых окон

• Запахи кода
  • Большие города методы
  • Сложные switch
  • Мёртвый код
  • Цепочки вызовов
• Теория разбитых окон
• Решение
• Заключение

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

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

В этой статье мы попробуем установить зависимость между недостаточной чистотой кода и наличием в нём ошибок. Поможет нам в этом статический анализатор PVS-Studio, который покажет все страшилки в коде.

Запахи кода

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

Большие города методы

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

Посмотрим на пример кода из проекта Akka.NET. Для начала взглянем с высоты птичьего полёта:

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

Как я и сказал, среди этих 102 строк действительно есть ошибка. Давайте на неё посмотрим:

....
MinNrOfMembersOfRole = clusterConfig.GetConfig("role")
                        .Root.GetObject().Items
                        .ToImmutableDictionary(
                         kv => kv.Key, 
                         kv => kv.Value.GetObject()  
                               .GetKey("....").GetInt()); <=
....

Предупреждения PVS-Studio:

V3080 [CWE-476] Possible null dereference of method return value. Consider inspecting: GetObject().

V3080 [CWE-476] Possible null dereference of method return value. Consider inspecting: GetObject().

Ошибка довольно простенькая: метод GetObject может вернуть нам нулевое значение, а мы не проверяем возможность возникновения такой ситуации. То есть буквально забыли поставить один символ ?. Однако обнаружить эту ошибку до того, как она стрельнёт, вряд ли получится. Из-за размеров этого метода ошибка с высокой долей вероятности навсегда останется похороненной под тонной незарефакторенного кода.

Сложные switch

Этот код "пахнет" проблемами с объектно-ориентированным проектированием. Использованию switch, кстати, была посвящена существенная часть дискуссии о чистом коде между Робертом Мартином и Кейси Муратори.

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

Столь же трудной задачей может быть обнаружение ошибок в больших (или не очень) switch и if выражениях. Причём эти ошибки могут быть связаны не только с проблемами объектно-ориентированного дизайна.

Вполне обычной ситуацией может быть пропуск одного из условий в switch, например, при использовании enum. На такой случай есть пример из проекта CUBA Platform:

enum Operation {
    REFRESH,
    CLEAR,
    ADD,
    REMOVE,
    UPDATE
}

@Override
public void setDatasource(final CollectionDatasource datasource) {
  ....
  collectionChangeListener = e -> {
    switch (e.getOperation()) {
      case CLEAR:
      case REFRESH:
        fieldDatasources.clear();
        break;

      case UPDATE:
      case REMOVE:
        for (Object entity : e.getItems()) {
          fieldDatasources.remove(entity);
        }
        break;
    }
  };
  ....
}

Предупреждение PVS-Studio: V6002 The switch statement does not cover all values of the 'Operation' enum: ADD.

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

Также есть вот такой пример из девятой версии .NET:

public static void SetAsIConvertible(this ref ComVariant variant,
                                     IConvertible value)
{
  TypeCode tc = value.GetTypeCode();
  CultureInfo ci = CultureInfo.CurrentCulture;

  switch (tc)
  {
    case TypeCode.Empty: break;
    case TypeCode.Object: 
      variant = ComVariant.CreateRaw(....); break;
    case TypeCode.DBNull: 
      variant = ComVariant.Null; break;
    case TypeCode.Boolean: 
      variant = ComVariant.Create<bool>(....)); break;
    case TypeCode.Char: 
      variant = ComVariant.Create<ushort>(value.ToChar(ci)); break;
    case TypeCode.SByte: 
      variant = ComVariant.Create<sbyte>(value.ToSByte(ci)); break;
    case TypeCode.Byte: 
      variant = ComVariant.Create<byte>(value.ToByte(ci)); break;
    case TypeCode.Int16: 
      variant = ComVariant.Create(value.ToInt16(ci)); break;
    case TypeCode.UInt16: 
      variant = ComVariant.Create(value.ToUInt16(ci)); break;
    case TypeCode.Int32: 
      variant = ComVariant.Create(value.ToInt32(ci)); break;
    case TypeCode.UInt32: 
      variant = ComVariant.Create(value.ToUInt32(ci)); break;
    case TypeCode.Int64: 
      variant = ComVariant.Create(value.ToInt64(ci)); break;
    case TypeCode.UInt64: 
      variant = ComVariant.Create(value.ToInt64(ci)); break;
    case TypeCode.Single: 
      variant = ComVariant.Create(value.ToSingle(ci)); break;
    case TypeCode.Double: 
      variant = ComVariant.Create(value.ToDouble(ci)); break;
    case TypeCode.Decimal: 
      variant = ComVariant.Create(value.ToDecimal(ci)); break;
    case TypeCode.DateTime: 
      variant = ComVariant.Create(value.ToDateTime(ci)); break;
    case TypeCode.String: 
      variant = ComVariant.Create(....); break;

    default:
      throw new NotSupportedException();
  }
}

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

....
case TypeCode.Int64: 
  variant = ComVariant.Create(value.ToInt64(ci)); break;
case TypeCode.UInt64: 
  variant = ComVariant.Create(value.ToInt64(ci)); break; // <=
....

Предупреждение PVS-Studio:

V3139 Two or more case-branches perform the same actions.

Если вы обратили внимание, то для типа UInt64 используется метод ToInt64. Вероятнее всего, это copy-paste ошибка. Но посмотрите, как надёжно огромный switch охраняет её от наших глаз...

Мёртвый код

Мёртвый код — фрагмент программы, который может быть исполнен, но никак не повлияет на результат её выполнения. Такой код может создавать переменные, которые также называются мёртвыми (они были созданы, но никогда не использовались).

Вот такой невероятно простенький пример:

int sum (int a, int b) {
  int result = a + b;
  return a + b;
}

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

А вот пример из проекта Universal:

template<typename Scalar>
vector<Scalar> operator*(double scalar, const vector<Scalar>& v) {
  vector<Scalar> scaledVector(v);
  scaledVector *= scalar;
  return v;
}

Предупреждение PVS-Studio: V1001 The 'scaledVector' variable is assigned but is not used by the end of the function.

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

В интерпретации Роберта Мартина, кстати, мёртвым также является и недостижимый код, то есть тот его фрагмент, который никогда не будет выполнен. Эта разница в понятиях также влияет и на возможные последствия возникновения такой ситуации в коде.

Посмотрим на пример из проекта Apache Dubbo:

@Override
public NextAction handleRead(FilterChainContext context) throws IOException {
  .... 
  do {
    savedReadIndex = frame.readerIndex();
    try {
      msg = codec.decode(channel, frame);
    } catch (Exception e) {
      previousData = ChannelBuffers.EMPTY_BUFFER;
      throw new IOException(e.getMessage(), e);
    }
    if (msg == Codec2.DecodeResult.NEED_MORE_INPUT) {
      frame.readerIndex(savedReadIndex);
        return context.getStopAction();     // <=
    } else {
      if (savedReadIndex == frame.readerIndex()) {
        previousData = ChannelBuffers.EMPTY_BUFFER;
        throw new IOException("Decode without read data.");   // <=
      }
      if (msg != null) {
        context.setMessage(msg);
        return context.getInvokeAction();     // <=
      } else {
        return context.getInvokeAction();    // <=
      }
    }
  } while (frame.readable());     // <=
  ....
}

Предупреждение PVS-Studio:

V6019 Unreachable code detected. It is possible that an error is present.

В этом do ̶ while происходит выход из метода во время выполнения do блока, поэтому вся while часть "отмирает". Этот фрагмент является примером недостижимого кода. И в здесь дело уже не только в чистоте кода, но и в неверной логике программы.

Вероятно, раз этот while был написан, то он должен был отрабатывать и выполнять определённые действия, однако в таком варианте до него дело не дойдёт.

А есть вот такой фрагмент из IntelliJ IDEA Community Edition:

private static void doTest(@Nullable JBCefClient client) {
  ....
  try {
    browser = new JCEFHtmlPanel(client, "about:blank");
  } catch (RuntimeException ex) {
    fail("Exception occurred: " + ex.getMessage());
    ex.printStackTrace();         <=
  }
  ....
}

Предупреждение PVS-Studio: V6019 Unreachable code detected. It is possible that an error is present.

Казалось бы, почему анализатор ругается на эту строчку? Немного терпения, друзья. Для того, чтобы раскрыть это дело, необходимо заглянуть внутрь метода fail:

public static void fail(String message) {
    if (message == null) {
        throw new AssertionError();
    }
    throw new AssertionError(message);
}

Эврика! Метод бросает исключение при каждом вызове, поэтому в doTest до метода ex.printStackTrace дело так и не дойдёт. Пусть эта проблема и возникает в модульном тесте, но увидеть подробную информацию об ошибке в случае его неудачного прохождения было бы полезно.

Цепочки вызовов

Цепочкой вызовов можно назвать ситуацию, когда клиент запрашивает у одного объекта другой, а тот запрашивает ещё один, и так до бесконечности.

Подобная ситуация имеет сразу несколько последствий:

• при изменении промежуточных связей нам потребуется изменить и клиент;
• цепочка вызовов требует больше времени, чтобы её прочитать и понять;
• лишняя трата производительности.

Предлагаю посмотреть на пример цепочки вызовов из проекта ReactOS:

....
w.NumChannels = AUD_BUF->audinfo().channels();
w.SampleRate = AUD_BUF->audinfo().sample_rate();
w.ByteRate = AUD_BUF->audinfo().byte_rate();
w.BlockAlign = AUD_BUF->audinfo().block_align();
w.BitsPerSample = AUD_BUF->audinfo().bits();
....

Предупреждение PVS-Studio: V807 Decreased performance. Consider creating a reference to avoid using the 'AUD_BUF->audinfo()' expression repeatedly.

Мы несколько раз обращаемся по одной и той же цепочке! Причём поправить такую ситуацию можно было бы довольно просто:

....
auto audinfo = AUD_BUF->audinfo();

w.NumChannels = audinfo.channels();
w.SampleRate = audinfo.sample_rate();
w.ByteRate = audinfo.byte_rate();
w.BlockAlign = audinfo.block_align();
w.BitsPerSample = audinfo.bits();
....

А вот такая красота была найдена нами в проекте Tizen:

for (size_t keypointNum = 0u;
    keypointNum < numberOfKeypoints; ++keypointNum)
{
    os << obj.__features.__objectKeypoints[keypointNum].pt.x << ' ';
    os << obj.__features.__objectKeypoints[keypointNum].pt.y << ' ';
    os << obj.__features.__objectKeypoints[keypointNum].size << ' ';
    os << obj.__features.__objectKeypoints[keypointNum].response << ' ';
    os << obj.__features.__objectKeypoints[keypointNum].angle << ' ';
    os << obj.__features.__objectKeypoints[keypointNum].octave << ' ';
    os << obj.__features.__objectKeypoints[keypointNum].class_id << '\n';
}

Предупреждение PVS-Studio: V807 Decreased performance. Consider creating a reference to avoid using the same expression repeatedly.

Проведя тот же "ритуал рефакторинга", что и в прошлый раз, мы получим вот такой код:

for (....) {
  auto &keypoint = obj.__features.__objectKeypoints[keypointNum];
  os << keypoint.pt.x << ' '
     << keypoint.pt.y << ' '
     << keypoint.size << ' '
     << keypoint.response << ' '
     << keypoint.angle << ' '
     << keypoint.octave << ' '
     << keypoint.class_id << '\n';
}

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

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

• Hide Delegate;
• Extract Method;
• Move Method.

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

Вот это да, чистый код помог производительности! Кто же мог себе такое представить?

Теория разбитых окон

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

И, как мне кажется, этот же принцип можно перенести на практику программной разработки.

Например, в сборочной системе падает сборка: одна из довольно большого списка. Если мы не отреагируем на это, то со временем в списке сборок будет уже больше красных значков, а ситуация с некорректной работой кода в сборочной среде станет вполне нормальной ситуацией: "Да, сборка упала из-за моего коммита, но посмотрите на остальные!"

Это же можно сказать и про качество кода. Допуская, с нашей точки зрения, незначительные ошибки, мы создаём прецедент, говорящий нам и окружающим, что допускать ошибки в коде можно. И с этого момента становится неважно, насколько критичными они являются.

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

public KeycloakUriBuilder schemeSpecificPart(String ssp)
  throws IllegalArgumentException {
    if (ssp == null) 
      throw new IllegalArgumentException(....);
    ....
    if (ssp != null)         // <=
      sb.append(ssp);
    ....
}

Предупреждение PVS-Studio: V6007 Expression 'ssp != null' is always true.

Здесь всё довольно просто: если переменная ssp будет равна null, то метод бросит IllegalArgumentException, а в обратном случае выполнение пойдёт далее. В этом фрагменте сравнение ssp != null лишнее.

И подобных срабатываний в этом проекте было довольно много. Однако, как вы понимаете, в проекте также были и другие интересные срабатывания анализатора. Например, вот такое:

public boolean equals(Object o) {
  if (this == o) return true;
  if (o == null || getClass() != o.getClass()) return false;

  Key key = (Key) o;

  if (action != key.action) return false;         // <=
  ....
}

Предупреждение PVS-Studio: V6013 Strings 'action' and 'key.action' are compared by reference. Possibly an equality comparison was intended.

В проверке, на которую указал анализатор, на самом деле сравнивается не содержимое двух строк, а ссылки на объекты. То есть эта проверка работает некорректно, и метод может завершать работу не так, как ожидал разработчик.

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

Решение

Что же может предложить в качестве решения разработчик статического анализатора?

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

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

Заключение

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

В статье я использовал примеры из различных open-source проектов, про некоторые из них мы писали ранее. Вот список статей с их проверками (в порядке появления):

• Святослав Размыслов. Анализ кода CUBA Platform с помощью PVS-Studio
• Артём Ровенский. Ошибки и подозрительные места в исходниках .NET 9
• Валерий Филатов. Неожиданности в авторизации. Что скрывает Keycloak?
• Валерий Комаров. Анализ исходного кода RPC фреймворка Apache Dubbo статическим анализатором PVS-Studio
• Кирилл Епифанов. Операция "K". Ищем баги в коде IntelliJ IDEA
• Тарас Шевченко. 11 мгновений ReactOS: user mode становится лучше?
• Андрей Карпов. 27000 ошибок в операционной системе Tizen

Чистого кода вам, друзья!