CollapsibleIfStatements

Question

I recently stumbled upon the following warning using PMD (built-in hudson), my code seems to suffer from CollapsibleIfStatements , which I don't quite understand. The code is as follows:

// 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 is no more “legible” (otherwise it will be even more unreadable for the next guy reading the code). On the other hand, I want to solve this warning (without deactivating PMD;).

Answer 1

One possibility is to split duplicate obj.getSomething() and then collapse the nested if :

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

In my opinion, the result is quite readable and should no longer trigger a PMD warning.

An alternative is to replace List with Set and throw an explicit contains() check in general. Using Set will also have better computational complexity.

Answer 2

I think this is what PMD wants:

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

Here are some suggestions for improvement (and don't let PMD scream):

• Extract variable:

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

• 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 a simple one:

   obj.getSomething().addToListIfNotEmpty(list); 

Answer 3

This is legible:

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

IMHO, this check may be useful sometimes, but it should be ignored in some other cases. The key should have the most readable code. If you think your code is more readable than a minimized if statement, add the @SuppressWarnings("PMD.CollapsibleIfStatements") annotation @SuppressWarnings("PMD.CollapsibleIfStatements") .