CollapsibleIfStatements

Question

Я недавно наткнулся на следующее предупреждение, используя PMD (встроенный в hudson), мой код, похоже, страдает CollapsibleIfStatements , что я не до конца понимаю. Код выглядит так

// list to be filled with unique Somethingness
List list = new ArrayList();

// fill list
for (SomeObject obj : getSomeObjects()) { // interating 
  if (!obj.getSomething().isEmpty()) { // check if "Something" is empty *
    if (!list.contains(obj.getSomething())) { // check if "Something" is already in my list **
      list.add(obj.getSomething()); // add "Something" to my list
    }
  }
}

По моему мнению, этот код не является более "разборным" (иначе он был бы еще более нечитаемым для следующего парня, читающего код). С другой стороны, я хочу решить это предупреждение (без деактивации PMD;).

Answer 1

Одна из возможностей - выделить повторяющиеся obj.getSomething(), а затем свернуть вложенные if операторы:

for (SomeObject obj : getSomeObjects()) {
  Something smth = obj.getSomething();
  if (!smth.isEmpty() && !list.contains(smth)) {
      list.add(smth);
  }
}

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

Альтернативой является замена List на Set и вообще исключение явных проверок contains(). Использование Set также будет иметь большую вычислительную сложность.

Answer 2

Разборно:

if (!obj.getSomething().isEmpty() && !list.contains(obj.getSomething())) {
    list.add(obj.getSomething());
}

ИМХО, эта проверка иногда может быть полезна, но в некоторых других случаях ее следует игнорировать. Ключ в том, чтобы код был как можно более читабельным. Если вы чувствуете, что ваш код более читабелен, чем свернутый оператор if, добавьте аннотацию @SuppressWarnings("PMD.CollapsibleIfStatements").

Answer 3

Я думаю, что именно этого хочет PMD:

if (!obj.getSomething().isEmpty() && !list.contains(obj.getSomething())) {
  list.add(obj.getSomething());
}

Вот несколько советов по улучшению (и предотвращению крика PMD):

1. Извлечение переменной:

   final Something sth = obj.getSomething();
   if (!sth.isEmpty() && !list.contains(sth)) {
     list.add(sth);
   }
   

2. Метод извлечения в Something (намного больше OOD):

   class Something {
   
       public void addToListIfNotEmpty(List<Something> list) {
           if(isEmpty() && list.contains(this)) {
               list.add(this);
           }
       }
   
   }
   

   и замените все условие на простое:

   obj.getSomething().addToListIfNotEmpty(list);