views:

568

answers:

13

Hello,

I always come across the same problem that when an exception is caught in a function that has a non-void return value I don't know what to return. The following code snippet illustrates my problem.

public Object getObject(){
  try{
    ...
    return object;
  }
  catch(Exception e){
    //I have to return something here but what??
    return null; // is this a bad design??
  }
}

So my questions are:

  • Is return null bad design?
  • If so what is seen as a cleaner solution??

thanks.

+7  A: 

Above all I prefer not to return null. That's something that the user has to explicitly remember to handle as a special case (unless they're expecting a null - is this documented). If they're lucky they'll deference it immediately and suffer an error. If they're unlucky they'll stick it in a collection and suffer the same problem later on.

I think you have two options:

  1. throw an exception. This way the client has to handle it in some fashion (and for this reason I either document it and/or make it checked). Downsides are that exceptions are slow and shouldn't be used for control flow, so I use this for exceptional circumstances (pun intended)
  2. You could make use of the NullObject pattern.

I follow a coding style in which I rarely return a null. If/when I do, that's explicitly documented so clients can cater for it.

Brian Agnew
NullObject is almost always to the way to go. If returning null is a problem, just don't return null.
Bedwyr Humphreys
+1 for the NullObject Pattern - probably the best way to handle the "null" case - unless people are expecting null of course! :)
aperkins
I'm intrigued that someone downvoted this. Any clue why ?
Brian Agnew
+30  A: 

I would say don't catch the exception if you really can't handle it. And logging isn't considered handling an error. Better to bubble it up to someone who can by throwing the exception.

If you must return a value, and null is the only sensible thing, there's nothing wrong with that. Just document it and make it clear to users what ought to be done. Have a unit test that shows the exception being thrown so developers coming after you can see what the accepted idiom needs to be. It'll also test to make sure that your code throws the exception when it should.

duffymo
"And logging isn't considered handling an error." -- I wish everybody knew...
Cassy
+2  A: 

It's your code and it's not bad solution. But if you share your code you Shoudn't use it because it can throw unexpected exception (as nullpointer one).

You can of course use

public Object getObject() throws Exception {}

which can give to parent function usable information and will warn that something bad can happen.

oneat
+13  A: 

I always come across the same problem that when an exception is caught in a function that has a non-void return value I don't know what to return.

If you don't know what to return, then it means that you don't know how to handle the exception. In that case, re-throw it. Please, don't swallow it silently. And please, don't return null, you don't want to force the caller of the code to write:

Foo foo = bar.getFoo();
if (foo != null) {
    // do something with foo
} 

This is IMHO a bad design, I personally hate having to write null-checks (many times, null is used where an exception should be thrown instead).

So, as I said, add a throws clause to the method and either totally remote the try/catch block or keep the try/catch if it makes sense (for example if you need to deal with several exceptions) and rethrow the exception as is or wrap it in a custom exception.

Related questions

Pascal Thivent
+1 for the same personal hate.
BalusC
+4  A: 

Exceptions denote exceptional cases. Assuming your code was supposed to return an object, something must have gone wrong on the way (network error, out of memory, who knows?) and therefore you should not just hush it by returning null.

However, in many cases, you can see in documentation that a method returns a null when such and such condition occurs. The client of that class can then count on this behaviour and handle a null returned, nothing bad about that. See, in this second usage example, it is not an exceptional case - the class is designed to return null under certain conditions - and therefore it's perfectly fine to do so (but do document this intended behaviour).

Thus, at the end of the day, it really depends on whether you can't return the object because of something exceptional in your way, or you simply have no object to return, and it's absolutely fine.

Peter Perháč
+2  A: 

Exceptions should always be caught by the controller in the end.
Passing a <null> up to the controller makes no sense.
Better to throw/return the original exception up the stack.

Kb
+2  A: 

I like the responses that suggest to throw an exception, but that implies that you have designed exception handling into the architecture of your software.

Error handling typically has 3 parts: detection, reporting, and recovery. In my experience, errors fall into classes of severity (the following is an abbreviated list):

  • Log for debug only
  • Pause whatever is going on and report to user, waiting for response to continue.
  • Give up and terminate the program with an apologetic dialogue box.

Your errors should be classified and handling should be as generically and consistently as possible. If you have to consider how to handle each error each time you write some new code, you do not have an effective error handling strategy for your software. I like to have a reporting function which initiates user interaction should continuation be dependent on a user's choice.

The answer as to whether to return a null (a well-worn pattern if I ever saw one) then is dependent on what function logs the error, what can/must the caller do if the function fails and returns the null, and whether or not the severity of the error dictates additional handling.

Bruce
+1  A: 

Some thoughts on how to handle Exceptions

  • Whether returning null would be good or bad design depends on the Exception and where this snippet is placed in your system.

  • If the Exception is a NullPointerException you probably apply the catch block somewhat obusive (as flow control).

  • If it is something like InOException and you can't do anything against the reason, you should throw the Exception to the controller.

  • If the controller is a facade of a component he, translate the Exception to well documented component-specific set of possible Exceptions, that may occur at the interface. And for detailed information include the original Exception as nested Exception.

stacker
+1  A: 

Basically I would ditto on Duffymo, with a slight addition:

As he says, if your caller can't handle the exception and recover, then don't catch the exception. Let a higher level function catch it.

If the caller needs to do something but should then appropriately die itself, just rethrow the exception. Like:

SomeObject doStuff()
  throws PanicAbortException
{
  try
  {
    int x=functionThatMightThrowException();
    ... whatever ...
    return y;
  }
  catch (PanicAbortException panic)
  {
    cleanUpMess();
    throw panic; // <-- rethrow the exception
  }
}

You might also repackage the exception, like ...

catch (PanicAbortException panic)
{
  throw new MoreGenericException("In function xyz: "+panic.getMessage());
}
Jay
I'd wonder if cleanup should always be done, then it'd be best to put it in a finally block and still not catch and re-throw. And I do not think that the more generic exception is adding much new information here.
duffymo
I was assuming in my example that "cleanup" meant some sort of cleanup after the exception, not a general cleanup that must be done regardless of whether the exception was thrown or not. That might mean logging the error, resetting counters, closing files, whatever.
Jay
I suppose my paragraph about repackaging the error was too sparse to add much to the discussion. What I had in mind was, I often have a function that catches several specific exceptions -- SQL errors, number format exceptions, etc -- and repackages them all into a general exception that it throws to its own caller. The reason being to not require all the specific exceptions to be declared and individually caught upstairs, because they are implementation-dependent and therefore not properly part of the interface.
Jay
+1  A: 

This is why so much java code is bloated with if (x!=null) {...} clauses. Don't create your own Null Pointer Exceptions.

sal
+1  A: 

As Josha Blooch says in the book "Effective Java", the null is a keyword of Java.

This word identifys a memory location without pointer to any other memory location: IMO it's better to coding with the separation of behaviour about the functional domain (example: you wait for an object of kind A but you receive an object of kind B) and behaviour of low-level domain (example: the unavaibility of memory).

I would coding your example:

public Object getObject(){
  Object objectReturned=new Object();
  try{
    /**business logic*/
  }
  catch(Exception e){
    //logging and eventual logic leaving the current ojbect (this) in a consistent state
  }
  return objectReturned;
}

The disavantage is to create a complete Object in every call of getObject() (then in situation where the objectReturned is not read or write).

But I prefer to have same object useless then a NullPointerException because sometimes this exception is very hard to fix.

alepuzio
+1  A: 

I would say it is a bad practice. If null is received how do I know if the object is not there or some error happened?

fastcodejava
+1  A: 

My suggestion is

Never ever return NULL if the writtern type is an array or Collection.Instead return an empty collection or an empty array

When the return type is an Object , it is upto you to return null depends on the scenario. But never ever swallow an Exception and return NULL.

Also if you are returning NULL in any scenario , ensure that this is documented in the menthod.

Sreejesh