3

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?

instanceOfObject
  • 2,936
  • 5
  • 49
  • 85
  • What do you do here: `if(o!=null){//do something} else {`? – assylias Mar 27 '12 at 17:27
  • I prefer to add checks all the time because it means that if I mess something up that _should_ be right -- if I accidentally pass null when I didn't mean to -- then it fails fast and immediately, so I notice sooner. – Louis Wasserman Mar 27 '12 at 17:32
  • Whatever. Suppose we access a member of object o and do some processing. Then? – instanceOfObject Mar 27 '12 at 17:32
  • 1
    Mike Samuel explained it quite well, so just a short answer from me to your specific cases: The null check does not seem helpful, although it is hard to say with such a small code fragment. However, it is important to think about what happens if o actually is null. Is it ok that do something is not executed and nobody is informed about this? To the second example: The check for totalPpl===0 makes perfect sense to me. The totalPpl – tim Mar 27 '12 at 17:35
  • 1
    @novice_at_work - @assylias is asking what you do if `o` *is* `null`--how do you handle the error? If all you do is throw an exception, why not let `NullPointerException` serve as-is? – David Harkness Mar 27 '12 at 17:36
  • @DavidHarkness oops! sorry! Yes, I agree to that! – instanceOfObject Mar 27 '12 at 17:37
  • @tim, I second the `totalPpl` check. `int`s can overflow/underflow in ways that references cannot in Java. A bad input -- 4 billion people -- could cause an `underflow` which could make `totalPpl` negative. – Mike Samuel Mar 27 '12 at 18:07
  • @MikeSamuel Yes, But then it should be reported in some other way. This check will hide that issue. Isn't it? – instanceOfObject Mar 27 '12 at 18:10
  • @novice_at_work, Agreed. You shouldn't just hide underflow by clamping to 0. – Mike Samuel Mar 27 '12 at 18:11

1 Answers1

8

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.

Community
  • 1
  • 1
Mike Samuel
  • 118,113
  • 30
  • 216
  • 245