3
\$\begingroup\$

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.

\$\endgroup\$
  • 3
    \$\begingroup\$ Don't worry about "pretty long", Code Review questions are often pretty long. If you share your original class we can probably help you better by seeing more patterns in your code. \$\endgroup\$ – Simon Forsberg Jun 10 '18 at 20:44
1
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Your first and last points are great but the catch (NullPointerException) is worthy of a downvote. Please don't recommend that approach! \$\endgroup\$ – Simon Forsberg Jun 10 '18 at 20:43
  • \$\begingroup\$ While I agree that catching a NPE is something I would flag in a code review, in some scenarios it can be the best option (dumb single run once utility program that either works or doesn't). Why make it an option if it should never be exercised? (P.S.: that downvote hurt! just starting out here :) ). \$\endgroup\$ – bowzerfood Jun 10 '18 at 21:09
  • \$\begingroup\$ @bowzerfood Catching a NullPointerException specifically like this just to ignore it is in my experience never ever the best option. \$\endgroup\$ – Simon Forsberg Jun 10 '18 at 22:25
  • \$\begingroup\$ @SimonForsberg What would be a better option in this case? I'd recommend at least coupling back to the user something has failed, but afar from that, for a single shot utility, I can at least understand the sentiment. There's an opportunity to teach at least 2 people at once here. \$\endgroup\$ – Mast Jun 10 '18 at 23:02
  • 2
    \$\begingroup\$ @Mast The short version: stackoverflow.com/a/18266490/1310566 . My main reasons: Creating and catching exception is immensely more expensive than checking for null. When you catch NPE you don't know which part in the chain/code was null - there is a risk of catching something you don't want to catch. \$\endgroup\$ – Simon Forsberg Jun 11 '18 at 14:54
0
\$\begingroup\$

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

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.