Is it better in Java to make all necessary checks from the caller or in the method it calls?

Question

Let's look at these two examples.

First:

try {
    execute(testObj);
} catch(Exception e) {
    //do somethingwith that
}

public void execute(TestObj testObj) throws Exception {
    if (testObj == null){
        throw new Exception("No such object");
    }
    //do something with object
}

Second:

if (testObj != null){
    execute(testObj);
} else {
    //handle this differently
}

public void execute(TestObj testObj) {
    //do something with object
}

It is not the point right now if we need to check "is null" or anything else. I want to know which practice is better in overall – "check, then do" or "do, then handle exception if occurs"?

Answer 1

Neither, you should only do checking on the boundaries between systems.

What is boundaries between system?

1. When you receive input from the user,
2. When you read a file, network, or socket,
3. When you do system calls or interprocess communication,
4. In library codes, in all public-facing APIs.
5. etc.

In library code, since public APIs may be called by external code, you should always do check in anything that can be called by external code. There is no need to do checking for an internal-only method (even if its access modifier is public). Argument errors may then be signalled by exception or by return code depending on personal taste, however checked exception cannot be ignored so it's usually the preferred method to signal errors to external codes.

In an application code (as opposed to a library), checking should only be done when receiving input from user, when you load a URL, when reading file, etc. There is no need to do input check in public methods because they are only ever called by your own application code.

Within your own code, you should avoid the situation where you need to check in the first place. For example, instead of checking for null, you can use "null object pattern". If you still need to do a check though, do it as early as possible, this usually means doing it outside but try to find even earlier points.

Even if not written down formally and even if it's not enforced, all methods whether its internal or public-facing should have a contract. The difference is that in external-facing code, you should enforce the contract as part of your error checking, while in internal-only code you can and should rely on the caller knowing what to and not to pass.

In short, since checking is only done on system boundaries, if you actually have a choice of whether to check inside or outside a method, then you probably should not be checking there since that is not a system boundary.

In a system boundary, both sides always have to check. The caller had to check that the external call succeeds and the callee had to check that the caller gives a sane input/argument. Ignoring an error in the caller is almost always a bug. Robustness principle applies, always check everything you receive from external system, but only send what you know that the external system can accept for sure.

In very big projects though, it's often necessary to compartmentalize the project into small modules. In this case, the other modules in the project can be treated as external systems and you should therefore do the checks in the APIs that are exposed to the other modules in the project.

TLDR; checking is hard.

Answer 2

As @LieRyan said, you should only do checking on the boundaries between systems. But once you are inside your own code, you still need to be able to detect unexpected problems, mostly because:

• you (and your co workers) are not perfect and may pass null to a method that can not handle it.
• if one line uses two objects and one is null the NPE will not tell you which one is to blame (System.out.println ("a.getName() " + "b.getName()") throws NPE...is a null or b null or both?)

Clearly document which methods accept null as parameters, or may return null.
A nice tool that can help you, if you are using Eclipse Juno, (I think IntelliJ-Idea has something similar) is to enable null checking analysis. It allow you to write some annotations to make compile time null checking. It is really awesome. You can write something like

public @NonNull String method(@Nullable String a){
//since a may be null, you need to make the check or code will not compile
    if(a != null){
        return a.toUppercase();        
    }
    return ""; //if you write return null it won't compile, 
     //because method is marked NonNull 
}

It also has a nice @NonNullByDefault, which basically says "No method accepts or returns null unless marked @Nullable" This is a nice default, keeps your code clean and safe.

For more info, check Eclipse help

Answer 3

I would say it depends on the type of exception. If the exception is down to invalid arguments, check them in the method. Otherwise, leave it to the caller to handle.

Example:

public void doSomething(MyObject arg1, MyObject arg2) throw SomeException {
   if (arg1==null || arg2==null) {
       throw new IllegalArgumentException(); //unchecked
   }
   // do something which could throw SomeException
   return;
}

calling:

try {
    doSomething(arg1, arg2);
} catch (SomeException e) {
    //handle
}