trjhtr

I'm working on an Spring Integration application that transforms a XML (converted by Jaxb) into a custom Object by checking various information given in the XML.

The problem is that I can't trust the data that has been sent to me and I have to do null checks everywhere. Can't decide if storing the result of a null check is better or is there another solution?

Solution 1: Do the null checks everywhere, and when we call another method, do the same null checks when needed. Which I dislike very much.

public void mapSpecificField(XmlObject xmlObject) 

 if (xmlObject != null) 
 if (xmlObject.getName() != null) 
 if (xmlObject.getName().equals("TYPE_A")) 
 if (xmlObject.getIdentifiers() != null 
 && !xmlObject.getIdentifiers().isEmpty()
 && xmlObject.getIdentifiers().get(0) != null) 
 String mainId = xmlObject.getIdentifiers().get(0).toLowerCase();

 if (xmlObject.getXmlSubObjects() != null) 
 for (XmlSubObject subObject : xmlObject.getXmlSubObjects()) 
 if (subObject != null && subObject.getLocation() != null) 
 mapOtherStuff(subObject);
 
 
 
 
 else 
 if (xmlObject.getIdentifiers() != null
 && !xmlObject.getIdentifiers().isEmpty()
 && xmlObject.getIdentifiers().get(0) != null) 
 String mainId = xmlObject.getIdentifiers().get(0).toUpperCase();
 
 
 
 

Solution 2: Do the null checks in an inner class and store the result in those variables. Which I like but not sure if memory wise it's a smart thing to do...

The inner helper class:

static class Helper {
 String type = null;
 String mainId = null;

 public Helper(XmlObject xmlObject) 
 if (xmlObject != null) 
 // Common
 if (xmlObject.getName() != null) 
 type = xmlObject.getName();
 // Specific mapping
 if (xmlObject.getIdentifiers() != null && !xmlObject.getIdentifiers().isEmpty() && xmlObject.getIdentifiers().get(0) != null) 
 if (type.equals("TYPE_A")) 
 mainId = xmlObject.getIdentifiers().get(0).toLowerCase();
 else 
 mainId = xmlObject.getIdentifiers().get(0).toUpperCase();
 
 
 
 
 

And the previous class using the helper:

public void mapSpecificField(XmlObject xmlObject) 
 Helper helper = new Helper(xmlObject);

 String mainId = helper.mainId;

 if ("TYPE_A".equals(helper.type)) 
 if (xmlObject.getXmlSubObjects() != null) 
 for (XmlSubObject subObject : xmlObject.getXmlSubObjects()) 
 if (subObject != null && subObject.getLocation() != null) 
 mapOtherStuff(subObject);
 
 
 
 

Or if anyone has a Solution 3, I'd be up for it as I really need some opinions...

Please do note that my original class has about 8 different types & lots of fields to map and I'm splitting the mapping into few methods that sometimes need the same variables. I can't share it here as it's pretty long but the classes above show exactly what it looks like.

I'd definitely go for the second version. If you are not in a very specific limited environment, the additional memory won't hurt. Note that you basically just copy the references to the concrete content, not the content itself, so the whole memory-signature-thing essentially revolves around a few 4-byte pointers.

As for the code itself: one way of coping with these long if a != null && a.getB() != null && a.getB().getC() != null chains is, to use Optional and its map function. In the end it's all matters of taste, but you might e.g. do:

// inner if from last code sample
Optional.ofNullable(subObject)
 .map(XmlSubObject::getLocation)
 .ifPresent(this::mapOtherStuff);

I like this style very much, but seriously: matters of taste.

One more thing: in similar situations I found it really helpful to include static utility functions which map from null to empty collection/list and vice versa:

public static <T> List<T> emptyListIfNull(List<T> l) 
 if(l == null)
 return new ArrayList<T>(); // or Collections.emptyList() if immutable is OK
 return l;


public static <T> List<T> nullIfEmpty(List<T> l) 
 if(l != null && !l.isEmpty())
 return l;
 return null;

That way, you can shorten expressions like

xmlObject.getIdentifiers() != null && !xmlObject.getIdentifiers().isEmpty()

to

nullIfEmpty(xmlObject.getIdentifiers()) != null

... which helps a lot in readability.

Edit: one more afterthought. As bowzerfood mentioned nulls and equals in his answer, I'd like to point out additionally, that today you can use Objects.equals(a, b) instead of a.equals(b) which handles nulls correctly and without resorting to third-party libraries.

Apache StringUtils has helpful static methods that mirror the String class but check for null as well. An extra library, but I always found it easier on the eyes than "foo".equals(bar).

StringUtils.isEmpty(null) // returns true

Kind of an unthought out idea, but have you considered something like:

try 
 mainId = xmlObject.getIdentifiers().get(0).toLowerCase();
 catch(NullPointerException npe) // ... 

Ugly, I know. But it might get to the point in an easier to read fashion. Also, unless you are working with an insane amount of objects (and even then) I wouldn't worry about performance here, just legibility and maintability.

There's a bit of simplification to be had in the way you chain if-conditions.

Consider the idea of "guard clause"s. These if-conditions help extracting useful information out of the state of an object, without adding a level of nesting. Basically they transform this code:

if (predicate(object)) 
 // use object

to a version that has less nesting:

if (!predicate(object)) 
 return;

// use object

This is of course not always possible, but in your case we can simplify both solutions with that:

if (xmlObject == null || xmlObject.getName() == null) 
 return;

// ...

This works especially well since java short-circuits boolean operators.

Let's talk a bit about the cost of memory associated with your Helper object.

The Helper has two object-typed fields. Each object field costs us a pointer (platform dependent, usually double-word -> 64-bit)

These pointers can point to objects that are already encapsulated in the xmlObject you maintain. Since the memory is already in use and we just reuse it, this adds no more overhead.

The memory cost of that helper is literally the same as having three local variables. (one additional pointer for the Helper instance itself).

Considering that I think it's highly preferrable to use the solution with the Helper object. You could even take it a step further and wrap the XmlSubObjects into the helper and make your mapSpecificField take a Helper instead of an XmlObject