Во время код-ревью или работы над новым проектом может раздражать многое: стиль, подходы, качество. Но больше всего расстраивает банальный недостаток гигиены кода. В связи с регулярностью проблемы хочется на неё обратить внимание и напомнить, как гигиену поддерживать.
Разумеется, все мы люди, живущие в несовершенном мире. Поэтому последнее, что мне хочется, это журить и так стеснённых обстоятельствами программистов. Тем более не хочется почём зря ругать разработчиков Open Source проектов. Но сейчас я предлагаю побыть перфекционистами и вспомнить все те случаи, когда с подобным кодом приходилось иметь дело вам.
Для воспроизведения ситуации просто возьмём случайный проект с GitHub, в данном случае — NGB, веб-приложение для просмотра геномов. А чтобы упростить поиск подозрительных мест в коде, используем статический анализатор PVS-Studio.
Ну так вот, нравится ли вам видеть код, который ничего не делает? Надеюсь, что да, иначе такое зрелище испортит вам настроение:
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 The return value of function 'map' is required to be utilized. GffManager.java(477)
Может показаться, будто здесь всё нормально. Переменная results объявляется, наполняется значениями и обрабатывается. Пока вы не увидите, что результат вызова map() не используется, а из метода возвращается совсем другое значение (его обработку я опустил для удобства чтения, каюсь). Влияет ли это негативно на работу программы либо просто осталось после рефакторинга — гадать вам (мы же представили, что это наш новый проект).
И вновь ситуация, где явно что-то идёт не так, только здесь оправдание придумать ещё сложнее:
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 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:
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:
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 The 'substring' function could receive the '-1' value while non-negative value is expected. Inspect argument: 2. VcfGa4ghReader.java(212)
V6009 The 'substring' function could receive the '-1' value while non-negative value is expected. Inspect argument: 2. TabixReader.java(430)
Ну или вот вам понадобилось подправить инфраструктуру DAO, заходите вы в нужный модуль, как видите этот метод:
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 The 'then' statement is equivalent to the 'else' statement. FeatureIndexDao.java(852), FeatureIndexDao.java(854)
Непонимание, грусть, разочарование. Судя по блейму, такая ситуация возникла при правке одной части кода разными разработчиками. Но даже когда мы нашли на кого свалить вину, ошеломление не пропадает: зачем кому-то понадобилось два условия подряд с одинаковыми ветвями? Или это я что-то не понял?
Есть большая вероятность, что неправильного поведения тут нет. Но а что, если есть? Остаётся только тыкать юнит-тесты...
Кстати, насчёт юнит-тестов. И к ним достаточно легко подорвать доверие. Например, допустить ошибку в их сетапе:
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(....):
public static String getRegistrationJsonWithPrettyName(
Long referenceID,
String path,
String name,
String indexPath,
boolean doIndex,
String prettyName) {
....
}Заметили? Если да, то можете отблагодарить за то, что я немного отформатировал код. Вот предупреждение:
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, то попробовать его можно, перейдя по этой ссылке.
А чтобы следить за выходом новых статей на тему качества кода, можете подписаться на: