CollapsibleIfStatements

Question

I recently stumpled upon the following warning using PMD (embedded in hudson), my code seems to suffer CollapsibleIfStatements, which I do not fully understand. The code looks like this

// 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
    }
  }
}

In my opinion this code isn't more "collapsible" (otherwise it would be even more unreadable for the next guy reading the code). On the other hand I want solve this warning (without deactivating PMD ;).

Answer 1

I think this is what PMD wants:

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

Here are few tips for improvements (and preventing PMD to scream):

1. Extract variable:

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

2. Extract method in Something (much more OOD):

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

   and replace the whole condition with simple:

   obj.getSomething().addToListIfNotEmpty(list);
   

Answer 2

It is collapsible:

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

IMHO, this check can be useful sometimes, but should be ignored in some other cases. The key is to have code as readable as possible. If you feel that your code is more readable than a collapsed if statement, then add a @SuppressWarnings("PMD.CollapsibleIfStatements") annotation.

Answer 3

One possibility is to factor out the repeated obj.getSomething() and then collapse the nested if statements:

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

To my eye, the result is quite readable, and should no longer trigger the PMD warning.

An alternative is to replace the List with a Set, and avoid the the explicit contains() checks altogether. Using a Set would also have better computational complexity.