CODEWITHSUNDEEP
javapmd
susho90
susho90

Reputation: 9

Deeply nested if statement remove PMD

How to remove below deeply nested if statement.

List<c1> list1 = new ArrayList();

    }
}

Upvotes: 0

Views: 2610

Answers (3)

Spotted
Spotted

Reputation: 4091

As @StephenC said, the best option is to avoid to provide null values so that you can remove these null checks. However, if for some reason you can't be sure that no null values are provided and you have to keep these null checks you can refactor you code using Optional:

return Optional.ofNullable(list2)
        .map(f -> getPIFObject(aModelManager, list2))
        .map(a -> a.getCoverageList().stream())
        .orElse(Stream.empty())
        .filter(c -> "eu".equalsIgnoreCase(c.getCoverageCd()) || "mn".equalsIgnoreCase(c.getCoverageCd()))
        .findFirst()
        .orElseThrow(() -> new CoverageException("No coverage found !")); //or orElse(defaultValue);

If you are the caller of this method, then ensure to never pass null (instead give empty lists/default values/null objects) then your code could be dramatically simplified to:

Asset listthree = (Asset) getPIFObject(aModelManager, list2);
for (Coverage abc : listthree.getCoverageList()) {
    if (("eu".equalsIgnoreCase(abc.getCoverageCd())) || ("mn".equalsIgnoreCase(abc.getCoverageCd()))) {
        return abc;
    }
}
throw new CoverageException("No coverage found !"); //or return a default value

This achieves the same behaviour as above but with Optional:

Asset listthree = (Asset) getPIFObject(aModelManager, list2);
return listthree.getCoverageList().stream()
        .filter(c -> "eu".equalsIgnoreCase(c.getCoverageCd()) || "mn".equalsIgnoreCase(c.getCoverageCd()))
        .findFirst()
        .orElseThrow(() -> new CoverageException("No coverage found !")); //or orElse(defaultValue);

Upvotes: 1

Stephen C
Stephen C

Reputation: 719436

I think your real problem is that you / your application are over-using null, either deliberately or because you have lots of problems with uninitialized variables and arrays.

If you tracked down and eliminated the source of those nulls, you could get rid of all of those null checks that are making your code complex. (In fact, I would be tempted to remove the checks anyway, and use the resulting NPEs to help you track down where the null values are coming from.)

To illustrate, this is what your code would look like if there were no nulls to worry about:

Asset listthree = (Asset) getPIFObject(aModelManager, list2);
List<c1> list1 = listthree.getCoverageList();
for (Coverage abc : list1) {
    if (("eu".equalsIgnoreCase(abc.getCoverageCd())) || 
        ("mn".equalsIgnoreCase(abc.getCoverageCd())) ) {
        myCoverage = abc;
        break;
    }
}

(I've also removed an unnecessary test to see if list1 is empty, and moved the declaration of list1.)

Upvotes: 3

David Lavender
David Lavender

Reputation: 8331

Move the code into new methods. The for-loop is a good candidate. Once it's in its own method, you can make it simpler too: by returning abc as soon as you find a match:

private String find(List<c1> list1) {
        for (Coverage abc :list1) {
            if(null != abc){
                if(("eu".equalsIgnoreCase(abc.getCoverageCd())) || ("mn".equalsIgnoreCase(abc.getCoverageCd())) ){
                    return abc;
                }
            }
        }
        return null;
}

Then just call it from your existing code:

list1 = listthree.getCoverageList();
myCoverage = find(list1);

Have also removed the if(!list1.isEmpty()){ check - that's not needed. If the list is empty, the for-loop will be fine: it just won't do anything.

The final thing you can do is to make sure you actually need all those null checks. If those values can never be null, then don't add the null check.

Upvotes: 2

Related Questions