Топ 10 ошибок в проектах Java за 2019 год

[GRaAL]

Автор метода findNextMenuItem хочет избавиться от -1, возвращаемой методом indexOf, в случае, если список menuItemWidgets не содержит currentItem

Не, тут дело похоже в другом.

Судя по названию метода «findNextMenuItem» и названию переменной «previousMenuItemFlatIndex» предположу что дело было так:

1. Был метод «findPreviousMenuItem» где было что-то вроде:
   
   

     int previousMenuItemFlatIndex = menuItemFlatIndex - 1;
     if (previousMenuItemFlatIndex >= 0) {                         
         return menuItemWidgets.get(previousMenuItemFlatIndex);
     }

2. Кто-то взял и скопировал этот метод, назвав его «findNextMenuItem»
3. Заменил " — 1" на " + 1" (чтобы взять следующий элемент вместо предыдущего), а все остальное — названия переменных и проверки — заменить забыл.
   

[Maccimo]

Картинки со звуком!
Сам хит-парад ошибок, увы, скучный. Копипаст и невнимательность.
Хотелось ошибок хотя бы такого уровня.
Кстати, PVS-Studio ошибку из статьи по ссылке найдёт?

[tagir_valeev]

Ну вот как раз подобный кейс со скоррелированным состоянием в том же методе, который разбирается на пятом месте (смотри мой комментарий ниже). Почему-то в хит-парад попал не он, а ложное срабатывание =)

[tagir_valeev]

 if (text == null || text.length() < 2) {
    return false;
  }
  if ("0".equals(text) || "0L".equals(text) || "0l".equals(text)) {// <=
    return false;
  }

Я ж вам про это говорил вроде. Это вообще не ошибка, просто перестраховка. Вы смотрите хоть немного логику метода. Да, если на входе строка "0", она отсекается двумя разными способами, в обоих случаях return false; произойдёт, поэтому без разницы, в какую ветку мы зайдём. Может сбить с толку, если не вчитаться в код, но ошибки здесь нет. Сама Идея этот код подсвечивает ещё с 2017-го года (и не заводите волынку, что раз мы его не исправили, то нищитово).

[tagir_valeev]

del

[tagir_valeev]

final int roundCarryMask = (1 << (bitShiftsInWord - 1));  // <=

Это тоже вообще не ошибка. Я потому не делаю подобные unsound-варнинги, потому что в них сбалансировать false-positive/false-negative очень сложно. Здесь откровенно мусорный варнинг, который только отвлечёт программистов от реальных проблем. Видите выше noRestore = bitShiftsInWord == 0? А ниже посмотрите на использования noRestore. Вы сразу увидите, что когда bitShiftsInWord == 0, результат битового сдвига (переменная roundCarryMask) не используется вообще. Поэтому абсолютно наплевать, какое там значение.

Почему-то вы, кстати, молчите про другую вещь в том же методе:

if (wordShifts == 0 && bitShiftsInWord == 0) {
    return;
}
...
final boolean noRestore = bitShiftsInWord == 0;
...
switch (wordShifts) {
case 0:
  // noRestore is always false
  roundCarry = (noRestore ? 0 : (this.v[0] & roundCarryMask)) != 0;
  ...

Здесь очевидно noRestore всегда ложно, потому что случай wordShifts == 0 && bitShiftsInWord == 0 был обработан выше. Идея радостно подсвечивает эти ветки и предлагает автоматически упростить код в один клик. PVS-Studio не может так? ;-)

[tagir_valeev]

В целом:

• Идея репортит 10, 9 (первый кейс), 8, 7, 6, 4.
• 5 репортить и не стоит, смотрите выше
• 9.2 репортить было бы хорошо, но это тоже unsound warning (а вдруг снаружи метода всегда проверяется, что x != index_64.length?). У меня была черновая реализация, но возникают помимо хороших варнингов реально очень мутные false-positive, где голову сломаешь перед тем как докажешь, что анализатор неправ. Я поэтому убрал этот код. Возможно, стоит вернуться.
• 3 должен репортиться инспекцией Integer multiplication or shift implicitly cast to long, но почему-то не срабатывает. Проверю, починю, спасибо!
• 2 — это интересная штука, у нас прямого аналога нет. Кое-какие циклы инициализации репортятся косвенно, но прямо такой нету.
• 1 — варнинг крутой, но как я понял инспекция у вас эвристическая. Возникает вопрос, сколько мусора она репортит. Вообще преаллоцированные массивы в toArray() в наши дни — антипаттерн. У нас на это есть инспекция, которая подсвечивает верхний по дефолту, но как раз молчит в нижнем, потому что вдруг пользователь реально хотел массив другой длины (чтобы были null'ы в хвосте).

[Antares1991]

А разве в пятом случае описанная ситуация не будет обработана строчкой assert (bitShiftsInWord >= 0);?

[ValeryKomarov]

assert бросит AssertionError в случае, если bitShiftsInWord будет отрицательным. При рассматриваемых входных данных (wordShifts = 3 и bitShiftsInWord = 0) assert промолчит, т.к. 0 >= 0 true =)