# Немного о гигиене Java кода

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

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

## Вместо вступления

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

Для воспроизведения ситуации просто возьмём случайный проект с GitHub, в данном случае — [NGB](https://github.com/epam/NGB), веб\-приложение для просмотра геномов\. А чтобы упростить поиск подозрительных мест в коде, используем статический анализатор PVS\-Studio\.

## Погружаемся

### Этому здесь не место

Ну так вот, нравится ли вам видеть код, который ничего не делает? Надеюсь, что да, иначе такое зрелище испортит вам настроение:

```cpp
public NggbIntervalTreeMap<Gene> loadGenesIntervalMap(GeneFile geneFile,
                                                      int startIndex,
                                                      int endIndex,
                                                      Chromosome chromosome) {
    ....
    List<Future<Boolean>> results;
    try {
        results = taskExecutorService.getExecutorService().invokeAll(callables);
    } catch (InterruptedException | AssertionError e) {
        throw new GeneReadingException(geneFile, 
                                       chromosome, 
                                       startIndex, 
                                       endIndex, 
                                       e);
    }

    results.stream().map(future -> {
        try {
            return future != null ? future.get() : null;
        } catch (InterruptedException | ExecutionException e) {
            log.error(getMessage(MessagesConstants.ERROR_GENE_BATCH_LOAD,
                                 geneFile.getId(), 
                                 chromosome.getId(), 
                                 e));
        }
        return null;
    });
    ....
    return genesRangeMap;
}
```

[V6010](https://pvs-studio.ru/ru/docs/warnings/v6010/) The return value of function 'map' is required to be utilized\. GffManager\.java\(477\)

Может показаться, будто здесь всё нормально\. Переменная _results_ объявляется, наполняется значениями и обрабатывается\. Пока вы не увидите, что результат вызова _map\(\)_ не используется, а из метода возвращается совсем другое значение \(его обработку я опустил для удобства чтения, каюсь\)\. Влияет ли это негативно на работу программы либо просто осталось после рефакторинга — гадать вам \(мы же представили, что это наш новый проект\)\.

### Это критичная ошибка?

И вновь ситуация, где явно что\-то идёт не так, только здесь оправдание придумать ещё сложнее:

```cpp
private static OrganismType determineHeterozygousGenotypeGA4GH(
    VariantGA4GH ga4GHEntity, List<Allele> alleles, int[] genotypeArray) {

    OrganismType organismType = null;
    for (int i = 0; i < genotypeArray.length; i++) {
        if (alleles.get(i).isReference()) {
            genotypeArray[i] = 0;
            organismType = OrganismType.HETEROZYGOUS;
        } else {
            if (organismType == null) {
                organismType = OrganismType.HETERO_VAR;
            }
            genotypeArray[i] = ga4GHEntity.getAlternateBases()
                                          .indexOf(alleles.get(i)) + 1; // <=
        }
    }

    return organismType;
}
```

[V6066](https://pvs-studio.ru/ru/docs/warnings/v6066/) The type of object passed as argument is incompatible with the type of collection: String, Allele\. VcfGa4ghReader\.java\(535\)

Метод_ ga4GHEntity\.getAlternateBases\(\)_ возвращает _List<String\>_, в то время как найти мы пытаемся объект типа _Allele_\. Так как объект другого типа, ожидаемо, найден не будет, в массив _genotypeArray_ будут записаны нули \(результат сложения \-1 и 1\)\. Пройдёте мимо, притворившись, что ничего не заметили, или попытаетесь исправить? Давление совести усиливается, если учесть, что эта и прошлая ошибки ловятся даже самим IntelliJ IDEA\.

### Ну как так\-то?

Продолжаем аки детективы \(неудачно\) выяснять, что же хотели разработчики\. Вот два забавных места\. Номер 1:

```cpp
private VariantSet variationMetadata(final String path) {
    final int index = path.indexOf('-');
    VariantSet variantSet;
    final String variantSetId;
    if (index >= 0) {
        variantSetId = path.substring(0, index - 1);
    } else {
        variantSetId = path;
    }
    ....
    return variantSet;
}
```

И номер 2:

```cpp
private TIntv getIntv(final String s) {
    ....
    int end = s.indexOf('\t', beg);
    while (end >= 0 || end == -1) {
        if (col == mSc) {
            ....
        } else {
            if ((mPreset & TYPE_FLAG) == 0) {
                if (col == mEc) {
                    intv.end = Integer.parseInt(end != -1 
                                                ? s.substring(beg, end) 
                                                : s.substring(beg));
                }
            } else if ((mPreset & TYPE_FLAG) == 1) {
                if (col == 6) {
                    String cigar = s.substring(beg, end);
                    ....
                }
            } else if ((mPreset & TYPE_FLAG) == 2) {
                String alt;
                alt = end >= 0 ? s.substring(beg, end) 
                               : s.substring(beg);
                ....
            }
        }
        if (end == -1) {
            break;
        }
        beg = end + 1;
        end = s.indexOf('\t', beg);
    }
    return intv;
}
```

Озаглавил этот раздел фразой, которую я бы сказал на код\-ревью, увидев такое\. Ошибку, кстати, я не привёл сразу специально — попробуйте найти сами \(это не трудно, я сократил код :\) \)\. Пока же оставлю свои мысли на этот счёт\.

После того как я покопался в проекте, у меня не появилось уверенности, что приложение легко уронить\. Но всё же сам факт допущения значения отрицательного индекса \(напрямую во втором случае и вследствие вычитания — в первом\) для извлечения подстроки вызывает фрустрацию\. Особенно в первом примере, где проверка индекса с извлечением подстроки буквально рядом\. А во втором случае есть ощущение, что был пропущен тернарный оператор, ибо в двух других местах он имеется\.

А вот и предупреждения на эти два участка кода:

[V6009](https://pvs-studio.ru/ru/docs/warnings/v6009/) The 'substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect argument: 2\. VcfGa4ghReader\.java\(212\)

[V6009](https://pvs-studio.ru/ru/docs/warnings/v6009/) The 'substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect argument: 2\. TabixReader\.java\(430\)

### Это так и задумано, или так и задумано?

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

```cpp
private String getGroupByField(final List<VcfFile> files, final String groupBy){
    final VcfIndexSortField sortField = VcfIndexSortField.getByName(groupBy);
    if (sortField == null) {
        ....
        if (   infoItem.getType() == VCFHeaderLineType.Integer
            || infoItem.getType() == VCFHeaderLineType.Float) {
            return infoItem.getName().toLowerCase();
        } else {
            return infoItem.getName().toLowerCase();
        }
    } else {
        if (   sortField.getType() == SortField.Type.INT
            || sortField.getType() == SortField.Type.FLOAT) {
            return sortField.getField().fieldName;
        } else {
            return sortField.getField().fieldName;
        }
    }
}
```

[V6004](https://pvs-studio.ru/ru/docs/warnings/v6004/) The 'then' statement is equivalent to the 'else' statement\. FeatureIndexDao\.java\(852\), FeatureIndexDao\.java\(854\)

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

![1086_ngb_ru/image2.png](https://import.viva64.com/docx/blog/1086_ngb_ru/image2.png)

Есть большая вероятность, что неправильного поведения тут нет\. Но а что, если есть? Остаётся только тыкать юнит\-тесты\.\.\. 

### Как тестировать юнит\-тесты

Кстати, насчёт юнит\-тестов\. И к ним достаточно легко подорвать доверие\. Например, допустить ошибку в их сетапе:

```cpp
public void addFeatureIndexedFileRegistration(Long refId,
                                              String path,
                                              String indexPath,
                                              String name,
                                              Long fileId,
                                              Long fileBioId, 
                                              BiologicalDataItemFormat format,
                                              boolean doIndex,
                                              String prettyName) {
    String absolutePath = getNormalizeAndAbsolutePath(path);
    onRequest().havingMethodEqualTo(HTTP_POST)
               .havingPathEqualTo(....)
               .havingBodyEqualTo(TestDataProvider
                                     .getRegistrationJsonWithPrettyName(
                                         refId, 
                                         absolutePath, 
                                         indexPath,
                                         name, 
                                         doIndex, 
                                         prettyName))
               .respond()
               .withBody(....)
               .withStatus(HTTP_STATUS_OK);
}
```

Чтобы раскусить подвох, надо посмотреть на сигнатуру метода: TestDataProvider\._getRegistrationJsonWithPrettyName\(\.\.\.\.\)_:

```cpp
public static String getRegistrationJsonWithPrettyName(
    Long referenceID,
    String path,
    String name,
    String indexPath,
    boolean doIndex,
    String prettyName) {
    ....
}
```

Заметили? Если да, то можете отблагодарить за то, что я немного отформатировал код\. Вот предупреждение:

[V6029](https://pvs-studio.ru/ru/docs/warnings/v6029/) Possible incorrect order of arguments passed to method: 'indexPath', 'name'\. TestHttpServer\.java\(139\), TestHttpServer\.java\(140\)

Судя по всему, при вызове метода перепутан порядок аргументов: порядок передачи переменных _indexPath_ и _name_ не соответствует порядку параметров с аналогичными именами в методе\. Компенсирован ли этот баг другим, или он где\-то негативно влияет на тесты, — остаётся только догадываться\. И радоваться, что править это всё же не нам\.

## Как следить за кодом

И вот такое попадается постоянно\. Хотя вроде всё понятно: хороший код писать, плохой — не писать, ошибаться не надо\. В следующий раз Капитан Очевидность вернётся к вам с новым удивительным открытием\.

Но если серьёзно, то все всё знают\. Как я и говорил, моё дело лишь напомнить\. Помимо банальной головы на плечах не надо игнорировать предупреждения IDE\. Далее можно подумать о линтерах для простых случаев и о специализированных инструментах для нахождения большего количества подозрительных мест\. Так, в этой статье использовался статический анализатор PVS\-Studio\. Но и автоматика, к сожалению, не панацея — простое человеческое код\-ревью никто не отменял\. Тут человека машина пока не заменит\.

## Послесловие

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

Если хочется самим проверить проект с помощью PVS\-Studio, то попробовать его можно, перейдя по [этой](https://pvs-studio.ru/ru/pvs-studio/try-free/) ссылке\.

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

* [Твиттер](https://twitter.com/Code_Analysis) PVS\-Studio;
* Наш ежемесячный [дайджест](https://pvs-studio.ru/ru/subscribe/) статей;
* Мой личный [блог](https://twitter.com/kvolokhovskii)\.