Is unnecessary error handling recommended in business logic? eg. Null check/Percentage limit check etc

Question

We usually put unnecessary checks in our business logic to avoid failures.

Eg.

1. public ObjectABC funcABC(){

      ObjectABC obj = new ObjectABC;
     ..........
     ..........
     //its never set to null here.
     ..........
     return obj; 
} 

ObjectABC o = funABC();

if(o!=null){
//do something
}

Why do we need this null check if we are sure that it will never be null? Is it a good practice or not?

2. int pplReached = funA(..,..,..);
   int totalPpl   = funB(..,..,..);

   funA() just puts a few more restriction over result of funB().


    Double percentage = (totalPpl==0||totalPpl<pplReached) ? 0.0 : pplReached/totalPpl;

Do we need 'totalPpl<pplReached' check?

The questions is: Aren't we swallowing some fundamental issue by putting such checks? Issues which should be shown ideally, are avoided by putting these checks.

What is the recommended way?

Answer 1

Think about your audience. A check is worthwhile when it

1. helps you, the programmer, detect errors,
2. helps other programmers detect errors where their code meets yours,
3. allows the program to recover from bad input or an invalid state, or
4. helps a maintainer avoid introducing errors later.

If your null check above does not fall into these, or there is a simpler mechanism which would do the same, then leave it out.

Simpler mechanisms often include,

1. unit tests.
2. annotations that communicate intent to the reader and can be checked by findbugs or similar tools
3. asserts that cause code to fail early, and communicate intent without requiring you to put in error handling code that should never be reached and without confusing code coverage tools
4. documentation or inline comments

In this case, I would suggest adding an annotation

public @Nonnull ObjectABC funcABC(){

integrating findbugs into your build process, and maybe replacing

if(o!=null){
//do something
}

with

assert o != null: "funcABC() should have allocated a new instance or failed."

Aren't we swallowing some fundamental issue by putting such checks?

As a rule of thumb,

1. unit tests are good for checking the behavior of a small piece of code. If you can't write unit tests for important functions, then the fundamental issue is that you aren't writing testable code.
2. annotations are good for conveying intent to code reviewers, maintainers, and automated tools. If you haven't integrated those tools into your process, then the fundamental issue is that you aren't taking advantage of available code quality tools.
3. asserts are good for double-checking your assumptions. If you can't sprinkle asserts into your code and quickly tell which are being violated then your fundamental problem is that you don't have a quick way to run your code against representative data to shake out problems.
4. documentation and inline comments (including source control comments) are good for spreading knowledge about the system among the team -- making sure that more than one person on the team can fix a problem in any part of the code. If they are constantly missing or out-of-sync, then the underlying problem is that you are not writing code with maintainers in mind.

Finally, design by contract is a programming methodology that many have found useful for business logic code. Even if you can't convince your team to adopt the specific tools and practices, reading up on DbC might still help you reason and explain how to enforce the important invariants in your codebase.