# Как PVS\-Studio ELKI в январе проверяли

Если вам кажется, что Новый год наступил только вчера, и вы не заметили, как прошла уже большая половина января – значит, все это время вы были заняты поиском трудноуловимых багов в поддерживаемом вами коде\. А также это значит, что наша статья именно для вас\. Мы, PVS\-Studio, проверили open source проект ELKI, чтобы показать вам, какие ошибки могут встретиться в коде, как хитро там они могут спрятаться, и как можно с этим бороться\.

![0791_ELKI_ru/image1.png](https://import.viva64.com/docx/blog/0791_ELKI_ru/image1.png)

## ELKI – что за библиотека?

Аббревиатура [ELKI](https://elki-project.github.io/) расшифровывается как **E**nvironment for Deve**l**oping **K**DD\-Applications Supported by **I**ndex\-Structures\. Этот проект написан на Java и предназначен для интеллектуального анализа данных\. Основные пользователи данной библиотеки – студенты, исследователи, специалисты по данным и инженеры\-программисты\. Это неудивительно, т\. к\. данная библиотека разрабатывалась как раз для проведения исследований\.

Алгоритмы, включенные в библиотеку, в основном, относятся к кластерному анализу и поиску выбросов, которые могут указывать на экспериментальные ошибки\. С каждым годом упоминаний об исследованиях, проведенных с использованием ELKI, становится все больше и больше\. На официальном сайте разработчиков можно найти целый [список](https://elki-project.github.io/references) научных работ, которые проводились с использованием данной библиотеки\. Из года в год этот список постоянно растет и дополняется, а темы научных работ становятся все разнообразнее\.

Единственным препятствием для использования данной библиотеки может стать AGPL лицензия, которая требует, чтобы полный исходный код приложения был открыт для любого пользователя\. Т\. е\. в коммерческих проектах с закрытым исходным кодом данную библиотеку использовать не получится\.

## Приступаем к проверке

[Библиотека](https://github.com/elki-project/elki/tree/474659eba15a27525db5f4c76162f28a7f1733aa) ELKI содержит 2 630 файлов на java, в которых 186 444 строки кода, не считая комментариев\. Поэтому проект кажется довольно маленьким на фоне некоторых open source гигантов\.

Однако небольшой размер библиотеки не является гарантией отсутствия в ней ошибок\. Как известно, ошибки могут быть в любых проектах вне зависимости от их масштаба\. В этом мы сейчас и убедимся\! Приступим к рассмотрению 10 самых интересных, на мой взгляд, предупреждений, обнаруженных при просмотре отчета\.

## Непроверенные границы

[V6001](https://pvs-studio.ru/ru/docs/warnings/v6001/) There are identical sub\-expressions 'bounds\[j \+ 1\]' to the left and to the right of the '\!\=' operator\. CLIQUEUnit\.java\(252\)

```cpp
private boolean checkDimensions(CLIQUEUnit other, int e) {
    for(int i = 0, j = 0; i < e; i++, j += 2) {
        if (dims[i] != other.dims[i]
            || bounds[j] != other.bounds[j]
            || bounds[j + 1] != bounds[j + 1]) {
          return false;
        }
    }
    return true;
}
```

В блоке _if_ метода _checkDimensions_ значение _bounds\[j \+ 1\]_ проверяется на неравенство со своим же собственным значением\. Естественно, данное условие всегда будет ложным, и одна из границ никогда не будет проверена\. То есть метод _checkDimensions_ может вернуть при проверке значение _true_ даже в том случае, если границы у массивов совпадать не будут\.

Корректная проверка в блоке _if_ должна выглядеть так:

```cpp
bounds[j + 1] != other.bounds[j + 1]
```

## Ленивый конструктор

[V6022](https://pvs-studio.ru/ru/docs/warnings/v6022/) Parameter 'updates' is not used inside constructor body\. DataStoreEvent\.java\(60\)

[V6022](https://pvs-studio.ru/ru/docs/warnings/v6022/) Parameter 'removals' is not used inside constructor body\. DataStoreEvent\.java\(60\)

```cpp
public DataStoreEvent(DBIDs inserts, DBIDs removals, DBIDs updates) {
    super();
    this.inserts = inserts;
    this.removals = inserts;
    this.updates = inserts;
}
```

На данный фрагмент кода было получено два предупреждения\. Здесь в конструктор _DataStoreEvent_ передаются три параметра, два из которых для инициализации не используются\. Если присмотреться к коду в конструкторе, становится понятно, что программист использовал копипаст и забыл исправить имена переменных, которые должны были участвовать в инициализации\.

Если пойти еще дальше, и рассмотреть класс _DataStoreEvent_ более детально, можно увидеть в нем три метода, которые активно используют вышеуказанный конструктор\.

```cpp
public static DataStoreEvent insertionEvent(DBIDs inserts) {
  return new DataStoreEvent(inserts, DBIDUtil.EMPTYDBIDS, DBIDUtil.EMPTYDBIDS);
}

public static DataStoreEvent removalEvent(DBIDs removals) {
  return new DataStoreEvent(DBIDUtil.EMPTYDBIDS, removals, DBIDUtil.EMPTYDBIDS);
}

public static DataStoreEvent updateEvent(DBIDs updates) {
  return new DataStoreEvent(DBIDUtil.EMPTYDBIDS, DBIDUtil.EMPTYDBIDS, updates);
}
```

Из\-за некорректной инициализации в конструкторе поведение данных методов тоже будет отличаться от ожидаемого\.

Корректный вариант кода в конструкторе должен быть таким:

```cpp
this.inserts = inserts;
this.removals = removals;
this.updates = updates;
```

## Непрозрачная переменная

[V6012](https://pvs-studio.ru/ru/docs/warnings/v6012/) The '?:' operator, regardless of its conditional expression, always returns one and the same value '0\.5'\. ClusterHullVisualization\.java\(173\), ClusterHullVisualization\.java\(173\)

```cpp
public void fullRedraw() {
    ....
    boolean flat = (clusters.size() == topc.size());
    // Heuristic value for transparency:
    double baseopacity = flat ? 0.5 : 0.5;
    ....
}
```

В этом фрагменте кода значение переменной _baseopacity_ всегда будет равно 0\.5 вне зависимости от того, какое значение будет у переменной _flat_\. Понятно, что здесь должны были быть два разных значения, но из\-за невнимательности автор кода забыл исправить одно из них\.

Ошибка простая и обидная\. Она сразу бросается в глаза, если из метода исключить строки кода, которые не имеют отношения к данной переменной\. Но обнаружить ее в методе, состоящем почти из 90 строк – непростая задача\. Похоже, что именно это и стало препятствием для программиста\.

## Получить запредельное

[V6025](https://pvs-studio.ru/ru/docs/warnings/v6025/) Index '1' is out of bounds\. GeneratorStatic\.java\(104\)

```cpp
@Override
public double[] computeMean() {
    // Not supported except for singletons.
    return points.size() == 1 ? points.get(1) : null;
}
```

В методе _computeMean_ автор кода проверяет, что размер коллекции _points_ равен единице, и, если это так, пытается вернуть из метода \.\.\. элемент коллекции с индексом 1\. Так как индексация начинается с нуля, исключения _IndexOutOfBoundsException_ здесь не избежать\.

Исправленный вариант кода должен выглядеть так:

```cpp
return points.size() == 1 ? points.get(0) : null;
```

## Можно ли делить на ноль?

[V6020](https://pvs-studio.ru/ru/docs/warnings/v6020/) Divide by zero\. The range of the 'referenceSetSize' denominator values includes zero\. PreDeConNeighborPredicate\.java\(138\)

```cpp
protected PreDeConModel computeLocalModel(DoubleDBIDList neighbors, ....) {
    final int referenceSetSize = neighbors.size();
    ....
    // Shouldn't happen:
    if(referenceSetSize < 0) {
        LOG.warning("Empty reference set – 
            should at least include the query point!");
        return new PreDeConModel(Integer.MAX_VALUE, DBIDUtil.EMPTYDBIDS);
    }
    ....
    for(int d = 0; d < dim; d++) {
        s[d] /= referenceSetSize;
        mvVar.put(s[d]);
    }
    ....
}
```

Каждый программист знает, что на ноль делить нельзя\. Каждый программист старается исключить появление такой ошибки в своем коде\. И в большинстве случаев это получается\. Но не всегда\.

В этом фрагменте кода все зависит от размера параметра _neighbors_, переданного в метод _computeLocalModel_\. Автор кода проверяет размер _neighbors_ и отсекает значения меньше нуля проверкой в операторе _if_, но при этом не предпринимает никаких действий, если размер равен нулю\. Т\.к\. проверка _referenceSetSize < 0_ не несет никакого смысла, и в сообщении для логирования тоже говорится про пустой _set_ – все это очень похоже на опечатку\. Скорее всего, здесь задумывалась проверка _referenceSetSize \=\= 0_\.

Если в данный метод все же будет передан пустой контейнер _neighbors_, то в цикле _for_ произойдет деление на ноль\. Остается надеяться, что такого действительно никогда не случится\.

## Бесконечная инициализация

[V6062](https://pvs-studio.ru/ru/docs/warnings/v6062/) Possible infinite recursion inside the 'setInitialMeans' method\. Predefined\.java\(65\), Predefined\.java\(66\)

```cpp
public void setInitialMeans(List<double[]> initialMeans) {
    this.setInitialMeans(initialMeans);
}
```

Этот метод из одной строки является самой настоящей рекурсией\. Сложно предположить, что именно хотел здесь написать автор кода\. Вероятно, единственная строка этого метода должна была выглядеть как\-то так:

```cpp
this.setInitialMeans(initialMeans.toArray(new double[0][0]));
```

Так как в этом классе есть еще один метод с таким же названием, программист, скорее всего, хотел передать данные для инициализации в него, но в итоге что\-то пошло не так\. Вот так, кстати, выглядит тело второго метода:

```cpp
public void setInitialMeans(double[][] initialMeans) {
    double[][] vecs = initialMeans.clone(); // TODO: deep copy?
    this.initialMeans = vecs;
}
```

## Потерянный вес

[V6094](https://pvs-studio.ru/ru/docs/warnings/v6094/) The expression was implicitly cast from 'int' type to 'double' type\. Consider utilizing an explicit type cast to avoid the loss of a fractional part\. An example: double A \= \(double\)\(X\) / Y;\. ProbabilityWeightedMoments\.java\(130\)

```cpp
public static <A> double[] alphaBetaPWM(...., final int nmom) {
    final int n = adapter.size(data);
    final double[] xmom = new double[nmom << 1];
    double aweight = 1. / n, bweight = aweight;
    for(int i = 0; i < n; i++) {
        ....
        for(int j = 1, k = 2; j < nmom; j++, k += 2) {
            xmom[k + 1] += val * (aweight *= (n - i - j + 1) / (n - j + 1));
            xmom[k + 1] += val * (bweight *= (i - j + 1) / (n - j + 1));
        }
    }
    return xmom;
}
```

В цикле _for_ данного фрагмента кода происходит неявное приведение выражения _\(n \- i \- j \+ 1\) / \(n \- j \+ 1\) _от типа _int_ к типу _double_\. Потеря точности в данном случае может быть совершенно различной: от нескольких цифр после запятой до полного обнуления веса, если число по модулю окажется меньше единицы\. Скорее всего, это не совсем ожидаемое поведение, учитывая, что массив _xmom_ имеет тип _double_\. Убедиться в том, что программист имел здесь в виду совсем другое, помогает выражение _\(n – i – j \+ 1\) / \(n – j \+ 1\)_\. Допустим_, n – j \+ 1_ равно _x_\. Тогда получаем выражение: _\(x – i\) / x_\. Данное выражение всегда будет давать 0 при целочисленном делении, если только мы не начнем уходить в отрицательные диапазоны\. Но т\. к\. значения _n_ в данном фрагменте кода всегда больше нуля, можно сделать вывод, что программист не собирался здесь делить целочисленно\.

Для того, чтобы избежать потери точности, необходимо явное преобразование к типу _double_:

```cpp
xmom[k + 1] += val * (aweight *= (double) (n - i - j + 1) / (n - j + 1));
xmom[k + 1] += val * (bweight *= (double) (i - j + 1) / (n - j + 1));
```

## Выход за границы

[V6079](https://pvs-studio.ru/ru/docs/warnings/v6079/) Value of the 'splitpoint' variable is checked after use\. Potential logical error is present\. KernelDensityFittingTest\.java\(97\), KernelDensityFittingTest\.java\(97\)

```cpp
public final void testFitDoubleArray() throws IOException {
    ....
    int splitpoint = 0;
    while(fulldata[splitpoint] < splitval && splitpoint < fulldata.length) {
        splitpoint++;
    }
    ....
}
```

В данном фрагменте кода в цикле _while_ сначала производится сравнение элемента массива _fulldata_ с индексом _splitpoint_ со значением переменной _splitval_, и только затем проверяется, что значение _splitpoint_ меньше, чем размер самого массива\. Две эти проверки в цикле _while_ нужно поменять местами, иначе можно очень легко оказаться за границами массива\.

## Недостижимый код

[V6019](https://pvs-studio.ru/ru/docs/warnings/v6019/) Unreachable code detected\. It is possible that an error is present\. Tokenizer\.java\(172\)

[V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'c \!\= '\\n'' is always true\. Tokenizer\.java\(169\)

```cpp
public String getStrippedSubstring() {
    int sstart = start, ssend = end;
    while(sstart < ssend) {
        char c = input.charAt(sstart);
        if(c != ' ' || c != '\n' || c != '\r' || c != '\t') {
            break;
        }
        ++sstart;
    }
    ....
}
```

На этот фрагмент кода заругались сразу две диагностики, которые в этот раз решили действовать сообща\. Диагностика V6019 указала на недостижимый фрагмент кода: _\+\+sstart_, а диагностика V6007 указала на условие в операторе _if_, которое всегда будет истинным\.

Почему в блоке _if_ всегда будет истина? Все очень просто\. В данном операторе проверяется сразу несколько условий: _c \!\= ' '_, или _c \!\= '\\n'_, или _c \!\= '\\r'_, или _c \!\= '\\t'_\. При любых входных данных какое\-нибудь из перечисленных условий будет истинным\. Даже если одна из проверок будет _false_, следующая проверка вернет _true_, и из\-за оператора _\|\|_ \(или\) условие в _if_ в итоге будет истинным\. А так как условие в блоке _if_ всегда будет истинным – будет срабатывать оператор _break_, досрочно завершающий цикл _while_, и инкремент переменной _sstart_ никогда не будет выполнен\. Именно это заметила диагностика V6019 и начала бить тревогу\.

Скорее всего, программист хотел написать что\-то вроде этого:

```cpp
if(c != ' ' && c != '\n' && c != '\r' && c != '\t')
```

## Переопределяй, но проверяй

[V6009](https://pvs-studio.ru/ru/docs/warnings/v6009/) Function 'equals' receives an odd argument\. An object 'other\.similarityFunction' is used as an argument to its own method\. AbstractSimilarityAdapter\.java\(91\)

```cpp
@Override
public boolean equals(Object obj) {
    if(obj == null) {
        return false;
    }

    if(!this.getClass().equals(obj.getClass())) {
        return false;
    }

    AbstractSimilarityAdapter<?> other = (AbstractSimilarityAdapter<?>) obj;
    return other.similarityFunction.equals(other.similarityFunction);
}
```

Автор кода решил переопределить метод _equals_ в классе _AbstractSimilarityAdapter_\. Если предполагается, что в программе объекты будут храниться в контейнерах, либо проверяться на равенство, переопределение _equals_ просто необходимо\. Однако, всю задумку автора испортила последняя строка метода, в которой _equals_ вызывается для того же самого объекта\. В итоге даже самое обычное сравнение будет происходить некорректно\.

Корректный код должен был выглядеть так:

```cpp
return this.similarityFunction.equals(other.similarityFunction);
```

Хочется заметить, что данный паттерн ошибок часто встречается в программах на разных языках, а не только в Java\. Об этом у нас есть статья '[Зло живет в функциях сравнения](https://habr.com/ru/company/pvs-studio/blog/329090)'\. Обязательно почитайте ее, если вам интересно узнать, почему же программисты часто допускают ошибки в достаточно простых функциях, предназначенных для сравнения двух объектов\. 

## Подведем итоги

Как видите, в любом проекте могут появиться ошибки\. Одни из них \- простые и обидные, другие – хитрые и коварные\. И не имеет никакого значения – маленький ваш проект, или большой\. Не имеет значения – профессиональный вы разработчик, или только начинаете писать код\. Ошибки могут появиться в любом месте вашей программы, и надежно спрятаться от ваших глаз\. 

Для каждого разработчика и для каждого проекта статический анализ кода – это незаменимый инструмент\. Такой же незаменимый, как review кода, или unit тесты\. Поэтому [прямо сейчас](https://pvs-studio.ru/ru/pvs-studio/download/) начинайте использовать статический анализ кода в своей работе, чтобы в Новом году и ваш код стал лучше и чище, чем в предыдущем\.

[![getTrialImageLink](https://cdn.pvs-studio.ru/media/get_trial_insert_ru.png)](https://pvs-studio.ru/ru/pvs-studio-download/)

## В заключение

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

Конечно, написать код без ошибок невероятно сложно, а на практике так вообще невозможно\. Однако, библиотеки, используемые как строительные кирпичики в научных, медицинских, исследовательских программах должны проверяться максимально тщательно\. Код должен быть чистым, насколько это вообще возможно\. Ведь каждая ошибка может иметь очень неприятные последствия в будущем\. По крайней мере, статический анализ для таких проектов крайне полезен\.

Данную тематику мы уже затрагивали в некоторых других наших статьях, поэтому, если этот вопрос оказался для вас интересным, вы можете ознакомиться с некоторыми из них:

1. [Проверка проекта Trans\-Proteomic Pipeline \(TPP\)](https://pvs-studio.ru/ru/blog/posts/cpp/0156/)\.
1. [Большой Калькулятор выходит из\-под контроля](https://pvs-studio.ru/ru/blog/posts/cpp/0212/)\.
1. [Можем ли мы доверять используемым библиотекам?](https://pvs-studio.ru/ru/blog/posts/cpp/0271/)
1. [Анализ кода ROOT \- фреймворка для анализа данных научных исследований](https://pvs-studio.ru/ru/blog/posts/cpp/0682/)\.
1. [NCBI Genome Workbench: научные исследования под угрозой](https://pvs-studio.ru/ru/blog/posts/cpp/0591/)\.