15

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.

A--C
  • 36,351
  • 10
  • 106
  • 92
user710818
  • 23,228
  • 58
  • 149
  • 207
  • 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. – Neil Jan 21 '13 at 15:52
  • 2
    If null's are rare do not check them, use exceptions to handle errors. – Leonidos Jan 21 '13 at 15:53
  • 1
    @Leonidos 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). – Andreas Fester Jan 21 '13 at 15:56
  • Are these objects all of the same type? – Kevin Bowersox Jan 21 '13 at 15:57
  • Would it be important to know which of them IS null if one would be? – Fildor Jan 21 '13 at 16:01

12 Answers12

15

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);
benez
  • 1,856
  • 22
  • 28
  • can you please explain what you mean by using "map() to chain these checks?". I don't understand. – likejudo Jul 17 '21 at 20:03
  • @likejudo the method `Optional.map(..)` will call the provided method reference in case a value is present, thus it serves indirectly as a null check – benez Jul 17 '21 at 23:29
5

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.

Dave Newton
  • 158,873
  • 26
  • 254
  • 302
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
}
Prathab K
  • 89
  • 5
  • 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](https://stackoverflow.com/a/45572220/3298121) – Roman Seleznov Aug 08 '17 at 16:09
3

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.

djechlin
  • 59,258
  • 35
  • 162
  • 290
  • Multiple exit points from a method will not make the code more clear. – gaborsch Jan 21 '13 at 16:05
  • 4
    @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. – Dave Newton Jan 21 '13 at 16:08
  • @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. – gaborsch Jan 21 '13 at 16:17
  • @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 `return`s 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. – djechlin Jan 21 '13 at 16:19
  • @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. – Dave Newton Jan 21 '13 at 16:20
  • @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. – gaborsch Jan 21 '13 at 16:37
  • @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. – Dave Newton Jan 21 '13 at 16:44
2

It's common problem for null references in java.

I prefer chaining with &&:

if (obj1 != null && obj1.method1() != null && obj1.method1().method2() != null)
mishadoff
  • 10,719
  • 2
  • 33
  • 55
  • 1
    This is good, but will not work for 10 methods, it's not possible to see wat the code is doing. – gaborsch Jan 21 '13 at 16:01
  • 7
    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 `&&`. – Nikita Kouevda Jan 21 '13 at 16:04
2

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

Community
  • 1
  • 1
Petr Mensik
  • 26,874
  • 17
  • 90
  • 115
1

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!

Fildor
  • 14,510
  • 4
  • 35
  • 67
  • 6
    Throwing and catching an exception is very heavy operation, don't do that. – gaborsch Jan 21 '13 at 15:55
  • If the exception is pretty unlikely to happen, it won't be much "heavier" than 10 null-checks each time ... – Fildor Jan 21 '13 at 15:56
  • 1
    This way you swallow `NullPointerException` that can be thrown by any chained method. – mishadoff Jan 21 '13 at 15:58
  • 1
    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? – partlov Jan 21 '13 at 15:58
  • 1
    Who said it is important? From what the OP sais, he just needs the chainmembers to not be null. – Fildor Jan 21 '13 at 15:59
  • 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. – gaborsch Jan 21 '13 at 16:00
  • 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. – Fildor Jan 21 '13 at 16:01
  • Catching is much better the branching.+1 – Arpit Jan 21 '13 at 16:02
  • 1
    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. – djechlin Jan 21 '13 at 16:03
  • 1
    @Arpit Could you please give reasons for your statement? – gaborsch Jan 21 '13 at 16:03
  • @djechlin 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. – Fildor Jan 21 '13 at 16:05
  • Setting one catch point is not that tough than checking for branch every time. – Arpit Jan 21 '13 at 16:06
  • Hey i'm not sure! because i use both. so it will be great if someone provide a better strong reason. – Arpit Jan 21 '13 at 16:07
  • @Fildor 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. – djechlin Jan 21 '13 at 16:07
  • @Arpit I would argue that, since null check is the simplest check. But you forget about the possible side-effects like unintendedly swallowing an exception. – gaborsch Jan 21 '13 at 16:09
  • 2
    http://stackoverflow.com/questions/3490770/what-is-faster-try-catch-or-if-else-in-java-wrt-performance reading this thread changes my opinion on speed.. – Arpit Jan 21 '13 at 16:09
  • @djechlin 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. – Fildor Jan 21 '13 at 16:11
  • 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 – Arpit Jan 21 '13 at 16:12
  • @Fildor - 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. – djechlin Jan 21 '13 at 16:14
  • 1
    @Arpit 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) ... – Fildor Jan 21 '13 at 16:20
  • 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. – David T. Oct 08 '13 at 23:00
1

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.

gaborsch
  • 15,408
  • 6
  • 37
  • 48
1
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

Community
  • 1
  • 1
Daniel Magnusson
  • 9,541
  • 2
  • 38
  • 43
1

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

Ashwin Jayaprakash
  • 2,168
  • 24
  • 29
0

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.

James Daily
  • 587
  • 6
  • 21
0

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>();
  • that answer made my day :D waiting for the enhanced version that allows parameters for the methods! – benez Apr 22 '21 at 15:58
  • as we had so much fun with this solution, would you reveal the internals of `Util.getClass(obj)` that is called within `isNullReflect(..)`? – benez Apr 22 '21 at 19:16