Is having a return statement just to satisfy syntax bad practice?

Question

Consider the following code:

public Object getClone(Cloneable a) throws TotallyFooException {      if (a == null) {         throw new TotallyFooException();     }     else {         try {             return a.clone();         } catch (CloneNotSupportedException e) {             e.printStackTrace();         }     }     //cant be reached, in for syntax     return null; } 

The return null; is necessary since an exception may be caught, however in such a case since we already checked if it was null (and lets assume we know the class we are calling supports cloning) so we know the try statement will never fail.

Is it bad practice to put in the extra return statement at the end just to satisfy the syntax and avoid compile errors (with a comment explaining it will not be reached), or is there a better way to code something like this so that the extra return statement is unnecessary?

Answer 1

A clearer way without an extra return statement is as follows. I wouldn't catch CloneNotSupportedException either, but let it go to the caller.

if (a != null) {     try {         return a.clone();     } catch (CloneNotSupportedException e) {         e.printStackTrace();     } } throw new TotallyFooException(); 

It's almost always possible to fiddle with the order to end up with a more straight-forward syntax than what you initially have.

Answer 2

It definitely can be reached. Note that you're only printing the stacktrace in the catch clause.

In the scenario where a != null and there will be an exception, the return null will be reached. You can remove that statement and replace it with throw new TotallyFooException();.

In general^*, if null is a valid result of a method (i.e. the user expects it and it means something) then returning it as a signal for "data not found" or exception happened is not a good idea. Otherwise, I don't see any problem why you shouldn't return null.

Take for example the Scanner#ioException method:

Returns the IOException last thrown by this Scanner's underlying Readable. This method returns null if no such exception exists.

In this case, the returned value null has a clear meaning, when I use the method I can be sure that I got null only because there was no such exception and not because the method tried to do something and it failed.

^*Note that sometimes you do want to return null even when the meaning is ambiguous. For example the HashMap#get:

A return value of null does not necessarily indicate that the map contains no mapping for the key; it's also possible that the map explicitly maps the key to null. The containsKey operation may be used to distinguish these two cases.

In this case null can indicate that the value null was found and returned, or that the hashmap doesn't contain the requested key.