How better refactor chain of methods that can return null in java?

Question

I have code like:

obj1 = SomeObject.method1();
if (obj1 != null) {
  obj2 = obj1.method2();
  if (obj2 != null) {
     obj3 = obj2.method3();
     if (obj3 != null) {
              ............


     return objN.methodM();

   }
  }
 }
....

I have near 10 steps. It seems very fragile and error prone. Is there a better way to check on null chained methods?

Thanks.

Dittography

Unfortunately it is fragile, and it's not because of the fact that it may return null. Chains like this should be avoided or you risk creating dependencies. – Chorizo

If null's are rare do not check them, use exceptions to handle errors. – Jehoash

@Jehoash It depends on the contract whether it is an error to return null (e.g. java.lang.System.console() can return null if no console is available). One thing I would change is to negate the checks and check if the result is null, and return from the method in that case. This avoids the deep nesting of scopes (which I find hard to read, usually). – Rubbery

Are these objects all of the same type? – Occult

Would it be important to know which of them IS null if one would be? – Elbertine

Answer 1

It's common problem for null references in java.

I prefer chaining with &&:

if (obj1 != null && obj1.method1() != null && obj1.method1().method2() != null)

Calorimeter

This is good, but will not work for 10 methods, it's not possible to see wat the code is doing. – Cranach

We don't know that the methods have no side effects, and to avoid calling them multiple times anyway, you should do if ((obj2 = obj1.method2()) != null && (obj3 = obj2.method3()) != null && ...) if you're going to chain with &&. – Sensual

Answer 2

You can use java.util.Optional.map(..) to chain these checks:

return Optional.ofNullable(SomeObject.method1())
        .map(SomeObject2::method2)
        .map(SomeObject3::method3)
        // ....
        .map(SomeObjectM::methodM)
        .orElse(null);

Curtin

can you please explain what you mean by using "map() to chain these checks?". I don't understand. – Marigolde

@Marigolde the method Optional.map(..) will call the provided method reference in case a value is present, thus it serves indirectly as a null check – Curtin

Answer 3

More context is necessary to answer this question well.

For example, in some cases I'd advocate breaking out the inner if statements into their own methods, following the "each method should do a single thing, completely and correctly." In this case, calling the method and checking for null is that single thing: if it's null, it returns (or throws, depending on your actual needs). If it isn't, it calls the next method.

Ultimately I suspect this is a design issue, though, the solution to which is unknowable without insight into the problem being solved.

As it stands, this single chunk of code requires deep knowledge of (what I suspect are) multiple responsibilities, meaning in almost all cases, new classes, new patterns, new interfaces, or some combination would be required to make this both clean, and understandable.

Hardhack

Answer 4

We can use Java8 Functional Interface approach.

@FunctionalInterface
public interface ObjectValue<V> {
    V get();
}

static <V> V getObjectValue(ObjectValue<V> objectValue)  {
    try {
        return objectValue.get();
    } catch (NullPointerException npe) {
        return null;
    }
}

Object obj = getObjectValue(() -> objectA.getObjectB().getObjectC().getObjectD());
if(Objects.nonNull(obj)) {
//do the operation
}

Fluctuant

Good approach but you do not need to create another interface, use the standard java.util.function.Supplier interface. You can see a full example here: link – Airglow

Answer 5

Write like

obj1 = SomeObject.method1();
if (obj1 == null) 
    return;
 obj2 = obj1.method2();
 if (obj2 == null) 
    return;

etc. As a C developer this is a very common paradigm and is extremely common. If it's not possible to convert your code to this flat flow then your code needs to be refactored in the first place, regardless of what language it exists in.

Replace return with whatever you are actually doing in the case where these fail, whether that be return null, throw an exception, etc. - you've omitted that part of your code but it should be the same logic.

Tameika

Multiple exit points from a method will not make the code more clear. – Cranach

@GaborSch It won't make it more complicated in this case, either: failing fast, and not forcing the code reader to skip over large chunks of code to see what happens if it isn't null, increases readability. – Hardhack

@DaveNewton That is one point I can accept, but generally it is better not to jump out early. If you just return null;, that's OK, but if there are more complex return statements like return new("blahblah", 12, methodX(obj.whatever())); it would just obfuscate the reader (try to find the differences between 10 similar lines). Also increases the chances for an error. – Cranach

@GaborSch I agree with last point, but 1) if OP is doing that in the rest of the code in the ..., it will still be clearer to use proposed style, and 2) if OP is doing that anyway, as noted the code needs to be refactored regardless of language/style. Most likely the returns should be replaced by throw IllegalArgumentException as the function had preconditions that couldn't be fulfilled, or log and return null if all of the code base is return null-oriented. – Tameika

@GaborSch I disagree that a generalization can be made. IMO it is much easier to read code if it's very clear, very early, what happens. If you have an over-complicated return statement, as in your example, I find it unlikely that (a) it could be constructed the same way at all return points, and (b) if it could, that the developer wouldn't create a local variable to hold that value for re-use. – Hardhack

@DaveNewton Yes, there is no general solution, but the OP asked in theoretical way. I also agree that in some cases the early exit is good, but in other cases it may not be good. So, IMHO there is no theoretical solution for the problem. – Cranach

@GaborSch Which is more or less what I said. Based on the OP's post, and the OP's return method, multiple returns seems clear and concise. My objection was to your assertion that this answer's code wasn't more clear; IMO it is, by quite a bit. YMMV. – Hardhack

Answer 6

I thinh this kind of question was already answered here. Especially see the second aswer about Null Object Pattern .

Whither

Answer 7

You can chain them and surround everything with a try/catch and catch the NPE.

Like this:

try
{
    Object result = SomeObject.method1().method2().methodN();
    return result;
}
catch(NullPointerException ex)
{
     // Do the errorhandling here.
}

Outside that I second @Neil's comment: Try to avoid that kind of chains in the first place.

EDIT:

Votes show that this is very disputed. I want to make sure it is understood, that I do not actually recommend this!

Proceeding like this has many sideeffects and should be generally avoided. I just threw it into discussion for OPs special situation, only as one way to achieve the goal if not otherwise possible!

If anyone feels he needs to do this: Please read the comments for possible pitfalls!

Elbertine

Throwing and catching an exception is very heavy operation, don't do that. – Cranach

If the exception is pretty unlikely to happen, it won't be much "heavier" than 10 null-checks each time ... – Elbertine

This way you swallow NullPointerException that can be thrown by any chained method. – Calorimeter

I really don't suggest this. How would you now in this case is exception because method2 returned null or methodN thrown exception because of some bug? – Mckown

Who said it is important? From what the OP sais, he just needs the chainmembers to not be null. – Elbertine

Even a try block imposes effect, the VM has to set up a catch point for the exception. But the main problem is that the methodM() can even throw an NPE. – Cranach

Again: So what? I know it is not the most efficient solution. But it is obviously a not perfect architecture anyway. Not using the Exception seems premature optimization to me. – Elbertine

Catching is much better the branching.+1 – Distraught

This will catch internal nullpointer exceptions, whether it be in libraries used by methodN's or in Java's String class. Those will then be handled the same as expected null returns. – Tameika

@Distraught Could you please give reasons for your statement? – Cranach

@Tameika Yes, it will catch all that. So what? Do you know there are expected null returns? I don't. From what the OP states in his question, this is the solution I would implement without knowing any further requirements. – Elbertine

Setting one catch point is not that tough than checking for branch every time. – Distraught

Hey i'm not sure! because i use both. so it will be great if someone provide a better strong reason. – Distraught

@Elbertine In nearly all application domains, array-out-of-bounds, etc. will be handled differently than internal method errors (or internal method expected behavior). Even if the handling is the same for them now, it is unmaintainable to assume this will always be the case. – Tameika

@Distraught I would argue that, since null check is the simplest check. But you forget about the possible side-effects like unintendedly swallowing an exception. – Cranach

#3491270 reading this thread changes my opinion on speed.. – Distraught

@Tameika I get your point and I can accept it for the general question. Nevertheless we'd have to ask the OP if it would be important in this case. – Elbertine

conditions which leads to some logic change must be handled using if else. otherwise they must be thrown. this is my idea.. but i'm ready to change it if better is available – Distraught

@Elbertine - not quite - this still forces the reader to sift through all library code and make sure null pointers aren't getting swallowed here, even if OP is relatively sure it works here. – Tameika

@Distraught You should avoid such code as shown in the question by using patterns ;) Of course people are right when they say, that using try/catch and excpetions require some heavy duty by the JVM. Especially when compared to a simple if(o==null) ... – Elbertine

Exception implies that something being null is INVALID. whereas some of the objects may very well be null and still be valid, depending on the use-case. for example, i could want my Integer object being null to indicate a special case. i DON"T want slow run-time handling to handle that. – Barkentine

Answer 8

Try to format this way:

obj1 = SomeObject.method1();
if (obj1 != null) {
   obj2 = obj1.method2();
}
if (obj2 != null) {
    obj3 = obj2.method3();
}
if (obj3 != null) {
          ............
}

if (objN != null) {
   return objN.methodM();
}
return null;

Don't forget to initialize all your objs to null.

Cranach

@Elbertine Nesting blocks are impossible to look through. – Cranach

That's correct. I just wanted to know if I miss anything outside that. – Elbertine

Answer 9

obj1 = SomeObject.method1();
if (obj1 == null) throw new IllegalArgumentException("...");

obj2 = obj1.method2();
if (obj2 == null) throw new IllegalArgumentException("...");

obj3 = obj2.method3();
if (obj3 == null) throw new IllegalArgumentException("...");

if (objN != null) {
   return objN.methodM();
}

Some more discussion here

Downtrodden

in the function that calls this function ;p – Downtrodden

@Arpit ftr this isn't just a joke, it's very likely the correct design. ("Likely" because if the entire code base is oriented toward returning null on error it's not great to be the one function relying on exceptions.) – Tameika

ya that's true. i'm laughing on your ';p' – Distraught

Answer 10

If you are on Java 8 or higher, consider Optional:

• Check chains of "get" calls for null
• How to avoid checking for null values in method chaining?

Assoil

Answer 11

If you want are so trusting of your source objects that you plan to chain six of them together, then just continue to trust them and catch exceptions when they are thrown - hopefully rarely.

However if you decide not to trust your source objects, then you have two choices: add compulsive "!= null" checks everywhere and don't chain their methods...

OR go back and change your source object classes and add better null handling at the root. You can do it by hand (with null checks inside setters, for example), or alternatively you can use the Optional class in Java 8 (or Optional in Google's Guava if you're not on Java 8), which offers an opinionated null-handling design pattern to help you react to unwanted nulls the moment they are introduced, rather than wait for some poor consumer to encounter them later on.

Damick

Answer 12

For getter method who has no parameter , try this:

Util.isNull(person, "getDetails().iterator().next().getName().getFullName()")

for most of the cases, it works well. Basically, it is trying to use java reflection to do the null check layer by layer, till it reach the last getter method , since we do a lot cacheing for the reflection, the code works well in production. Please check the code below.

public static boolean isNull(Object obj, String methods) {
    if (Util.isNull(obj)) {
        return true;
    }
    if (methods == null || methods.isEmpty()) {
        return false;
    }
    String[] A = methods.split("\\.");
    List<String> list = new ArrayList<String>();
    for (String str : A) {
        list.add(str.substring(0, str.indexOf("(")).trim());
    }
    return isNullReflect(obj, list);
}
public static boolean isNullReflect(Object obj, List<String> methods) {
    if (Util.isNull(obj)) {
        return true;
    }
    if (methods.size() == 0) {
        return obj == null;
    }
    Class<?> className = Util.getClass(obj);
    try {
        Method method = Util.getMethod(className.getName(), methods.remove(0), null, className);
        method.setAccessible(true);
        if (method.getName().equals("next")
                && !Util.isNull(Util.getMethod(className.getName(), "hasNext", null, className))) {
            if (!((Iterator<?>) (obj)).hasNext()) {
                return true;
            }
        }
        try {
            return isNullReflect(method.invoke(obj), methods);
        } catch (IllegalAccessException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        } catch (IllegalArgumentException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        } catch (InvocationTargetException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
    } catch (SecurityException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }
    return false;
}


public static Boolean isNull(Object object) {
    return null == object;
}

public static Method getMethod(String className, String methodName, Class<?>[] classArray, Class<?> classObj) {
    // long a = System.nanoTime();
    StringBuilder sb = new StringBuilder();
    sb.append(className);
    sb.append(methodName);
    if (classArray != null) {
        for (Class<?> name : classArray) {
            sb.append(name.getName());
        }
    }
    String methodKey = sb.toString();
    Method result = null;
    if (methodMap.containsKey(methodKey)) {
        return methodMap.get(methodKey);
    } else {
        try {
            if (classArray != null && classArray.length > 0) {
                result = classObj.getMethod(methodName, classArray);
            } else {
                result = classObj.getMethod(methodName);
            }
            methodMap.put(methodKey, result);
        } catch (NoSuchMethodException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        } catch (SecurityException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
    }
    // long b = System.nanoTime();
    // counter += (b - a);
    return result;
}
    private static Map<String, Method> methodMap = new ConcurrentHashMap<String, Method>();

Nunn

that answer made my day :D waiting for the enhanced version that allows parameters for the methods! – Curtin

as we had so much fun with this solution, would you reveal the internals of Util.getClass(obj) that is called within isNullReflect(..)? – Curtin