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

StackOverflow https://stackoverflow.com/questions/14442299

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.

Was it helpful?

Solution 5

It's common problem for null references in java.

I prefer chaining with &&:

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

OTHER TIPS

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.

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
}

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);

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.

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

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.

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

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!

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.

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>();
Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top