1

Suppose I want to access something like

productList.get(0).getCustomerList().get(0).getAddressList().get(0).getRegion.getCode()

And at each level I need to check for null or empty list. The data structure is really complicated, and refactoring could be an option, but could as well be too complicated or impossible.

The resulting code would be:

if(productList != null 
   && !productList.isEmpty() 
   && productList.get(0).getCustomerList().get(0) != null 
   && ...){
  return productList.get(0).getCustomerList().get(0).getAddressList(0).getRegion.getCode();
}

The resulting code is ugly, verbose, doesn't carry any real business logic and is hard to read. Is there some smart way to avoid that? Would it be acceptable to do:

try{
  return productList.get(0).getCustomerList().get(0).getAddressList(0).getRegion.getCode();
} catch(NullPointerException | IndexOutOfBoundException e){
  return null;
}
Paolo
  • 2,461
  • 5
  • 31
  • 45
  • 2
    https://en.wikipedia.org/wiki/Law_of_Demeter ... – Tom Apr 13 '16 at 11:46
  • a teaser: `for {product <- productList.headOption; customer <- product.getCustomerList.headOption; address <- customer.getAddressList.headOption } yield {address.getRegion.getCode}` This will yield an `Option[Int]` containing the code or `None` if somewhere down the path there was a missing element. Eliminates the risks of `NullPointerException`. Scala and Functional Programming are your friends. – maasg Apr 13 '16 at 11:54
  • 1
    Side note: Lists and other enumerables should almost never be null anyways. Empty, yes, null, no. – Clockwork-Muse Apr 13 '16 at 12:03
  • Are you using Java-8? – reto Apr 13 '16 at 13:34

2 Answers2

2

I'd just split it into something like this:

Product p = getElementOrNull( productList, 0 );
if( p == null ) { return null; }

Customer c = getElementOrNull( p.getCustomerList(), 0 );
if( c == null ) { return null; }

Address a = getElementOrNull( c.getAddressList(), 0 );
if( a == null ) { return null; }   

Region r = a.getRegion();
if( r == null ) { return null; }   

return r.getCode();

with

<T> T getElementOrNull( List<T> list, int index ) {
  if( list == null || index < 0 || index >= list.size() ) {
    return null;
  }      

  return list.get( index );
}

Using exceptions to handle normal code flow like you suggest normally isn't adviced. You sometimes see it done that way and in some cases it would work but it makes the code harder to understand (what might be null or which index might be wrong) and might confuse less experiences developers that stumble upon your code and conclude it's ok to use exceptions that way in other cases.

In your case you seem to be ok with any element/list in the chain being null but assume cases where an address must always have a region or a customer must always have an address. In those cases you could just operate on the assumption that those things are not null and adapt the null checks to throw a more descriptive exception (at least with a better message) - which you couldn't do in a "catch-all" catch block like yours.

Thomas
  • 87,414
  • 12
  • 119
  • 157
1

Here is another suggestion by using Java 8 Optional.

Predicate<List> hasElement = list -> list != null && !list.isEmpty();

String code = Optional.ofNullable(productList)
.filter(hasElement).map(p -> p.get(0).getCustomerList())
.filter(hasElement).map(c -> c.get(0).getAddressList())
.filter(hasElement).map(a -> a.get(0).getRegion())
.map(Region::getCode)
.orElse(null);
simdevmon
  • 673
  • 7
  • 14