Java – Is having a return statement just to satisfy syntax bad practice

javareturntry-catch

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?

Best Answer

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.

Related Solutions

Java – Why is using a wild card with a Java import statement bad

The only problem with it is that it clutters your local namespace. For example, let's say that you're writing a Swing app, and so need java.awt.Event, and are also interfacing with the company's calendaring system, which has com.mycompany.calendar.Event. If you import both using the wildcard method, one of these three things happens:

You have an outright naming conflict between java.awt.Event and com.mycompany.calendar.Event, and so you can't even compile.
You actually manage only to import one (only one of your two imports does .*), but it's the wrong one, and you struggle to figure out why your code is claiming the type is wrong.
When you compile your code there is no com.mycompany.calendar.Event, but when they later add one your previously valid code suddenly stops compiling.

The advantage of explicitly listing all imports is that I can tell at a glance which class you meant to use, which simply makes reading the code that much easier. If you're just doing a quick one-off thing, there's nothing explicitly wrong, but future maintainers will thank you for your clarity otherwise.

Java – The case against checked exceptions

I think I read the same Bruce Eckel interview that you did - and it's always bugged me. In fact, the argument was made by the interviewee (if this is indeed the post you're talking about) Anders Hejlsberg, the MS genius behind .NET and C#.

http://www.artima.com/intv/handcuffs.html

Fan though I am of Hejlsberg and his work, this argument has always struck me as bogus. It basically boils down to:

"Checked exceptions are bad because programmers just abuse them by always catching them and dismissing them which leads to problems being hidden and ignored that would otherwise be presented to the user".

By "otherwise presented to the user" I mean if you use a runtime exception the lazy programmer will just ignore it (versus catching it with an empty catch block) and the user will see it.

The summary of the argument is that "Programmers won't use them properly and not using them properly is worse than not having them".

There is some truth to this argument and in fact, I suspect Goslings motivation for not putting operator overrides in Java comes from a similar argument - they confuse the programmer because they are often abused.

But in the end, I find it a bogus argument of Hejlsberg's and possibly a post-hoc one created to explain the lack rather than a well thought out decision.

I would argue that while the over-use of checked exceptions is a bad thing and tends to lead to sloppy handling by users, but the proper use of them allows the API programmer to give great benefit to the API client programmer.

Now the API programmer has to be careful not to throw checked exceptions all over the place, or they will simply annoy the client programmer. The very lazy client programmer will resort to catch (Exception) {} as Hejlsberg warns and all benefit will be lost and hell will ensue. But in some circumstances, there's just no substitute for a good checked exception.

For me, the classic example is the file-open API. Every programming language in the history of languages (on file systems at least) has an API somewhere that lets you open a file. And every client programmer using this API knows that they have to deal with the case that the file they are trying to open doesn't exist. Let me rephrase that: Every client programmer using this API should know that they have to deal with this case. And there's the rub: can the API programmer help them know they should deal with it through commenting alone or can they indeed insist the client deal with it.

In C the idiom goes something like

  if (f = fopen("goodluckfindingthisfile")) { ... } 
  else { // file not found ...

where fopen indicates failure by returning 0 and C (foolishly) lets you treat 0 as a boolean and... Basically, you learn this idiom and you're okay. But what if you're a noob and you didn't learn the idiom. Then, of course, you start out with

   f = fopen("goodluckfindingthisfile");
   f.read(); // BANG!

and learn the hard way.

Note that we're only talking about strongly typed languages here: There's a clear idea of what an API is in a strongly typed language: It's a smorgasbord of functionality (methods) for you to use with a clearly defined protocol for each one.

That clearly defined protocol is typically defined by a method signature. Here fopen requires that you pass it a string (or a char* in the case of C). If you give it something else you get a compile-time error. You didn't follow the protocol - you're not using the API properly.

In some (obscure) languages the return type is part of the protocol too. If you try to call the equivalent of fopen() in some languages without assigning it to a variable you'll also get a compile-time error (you can only do that with void functions).

The point I'm trying to make is that: In a statically typed language the API programmer encourages the client to use the API properly by preventing their client code from compiling if it makes any obvious mistakes.

(In a dynamically typed language, like Ruby, you can pass anything, say a float, as the file name - and it will compile. Why hassle the user with checked exceptions if you're not even going to control the method arguments. The arguments made here apply to statically-typed languages only.)

So, what about checked exceptions?

Well here's one of the Java APIs you can use for opening a file.

try {
  f = new FileInputStream("goodluckfindingthisfile");
}
catch (FileNotFoundException e) {
  // deal with it. No really, deal with it!
  ... // this is me dealing with it
}

See that catch? Here's the signature for that API method:

public FileInputStream(String name)
                throws FileNotFoundException

Note that FileNotFoundException is a checked exception.

The API programmer is saying this to you: "You may use this constructor to create a new FileInputStream but you

a) must pass in the file name as a String
b) must accept the possibility that the file might not be found at runtime"

And that's the whole point as far as I'm concerned.

The key is basically what the question states as "Things that are out of the programmer's control". My first thought was that he/she means things that are out of the API programmers control. But in fact, checked exceptions when used properly should really be for things that are out of both the client programmer's and the API programmer's control. I think this is the key to not abusing checked exceptions.

I think the file-open illustrates the point nicely. The API programmer knows you might give them a file name that turns out to be nonexistent at the time the API is called, and that they won't be able to return you what you wanted, but will have to throw an exception. They also know that this will happen pretty regularly and that the client programmer might expect the file name to be correct at the time they wrote the call, but it might be wrong at runtime for reasons beyond their control too.

So the API makes it explicit: There will be cases where this file doesn't exist at the time you call me and you had damn well better deal with it.

This would be clearer with a counter-case. Imagine I'm writing a table API. I have the table model somewhere with an API including this method:

public RowData getRowData(int row)

Now as an API programmer I know there will be cases where some client passes in a negative value for the row or a row value outside of the table. So I might be tempted to throw a checked exception and force the client to deal with it:

public RowData getRowData(int row) throws CheckedInvalidRowNumberException

(I wouldn't really call it "Checked" of course.)

This is bad use of checked exceptions. The client code is going to be full of calls to fetch row data, every one of which is going to have to use a try/catch, and for what? Are they going to report to the user that the wrong row was sought? Probably not - because whatever the UI surrounding my table view is, it shouldn't let the user get into a state where an illegal row is being requested. So it's a bug on the part of the client programmer.

The API programmer can still predict that the client will code such bugs and should handle it with a runtime exception like an IllegalArgumentException.

With a checked exception in getRowData, this is clearly a case that's going to lead to Hejlsberg's lazy programmer simply adding empty catches. When that happens, the illegal row values will not be obvious even to the tester or the client developer debugging, rather they'll lead to knock-on errors that are hard to pinpoint the source of. Arianne rockets will blow up after launch.

Okay, so here's the problem: I'm saying that the checked exception FileNotFoundException is not just a good thing but an essential tool in the API programmers toolbox for defining the API in the most useful way for the client programmer. But the CheckedInvalidRowNumberException is a big inconvenience, leading to bad programming and should be avoided. But how to tell the difference.

I guess it's not an exact science and I guess that underlies and perhaps justifies to a certain extent Hejlsberg's argument. But I'm not happy throwing the baby out with the bathwater here, so allow me to extract some rules here to distinguish good checked exceptions from bad:

Out of client's control or Closed vs Open:

Checked exceptions should only be used where the error case is out of control of both the API and the client programmer. This has to do with how open or closed the system is. In a constrained UI where the client programmer has control, say, over all of the buttons, keyboard commands etc that add and delete rows from the table view (a closed system), it is a client programming bug if it attempts to fetch data from a nonexistent row. In a file-based operating system where any number of users/applications can add and delete files (an open system), it is conceivable that the file the client is requesting has been deleted without their knowledge so they should be expected to deal with it.
Ubiquity:

Checked exceptions should not be used on an API call that is made frequently by the client. By frequently I mean from a lot of places in the client code - not frequently in time. So a client code doesn't tend to try to open the same file a lot, but my table view gets RowData all over the place from different methods. In particular, I'm going to be writing a lot of code like
```
if (model.getRowData().getCell(0).isEmpty())
```

and it will be painful to have to wrap in try/catch every time.

Informing the User:

Checked exceptions should be used in cases where you can imagine a useful error message being presented to the end user. This is the "and what will you do when it happens?" question I raised above. It also relates to item 1. Since you can predict that something outside of your client-API system might cause the file to not be there, you can reasonably tell the user about it:
```
"Error: could not find the file 'goodluckfindingthisfile'"
```
Since your illegal row number was caused by an internal bug and through no fault of the user, there's really no useful information you can give them. If your app doesn't let runtime exceptions fall through to the console it will probably end up giving them some ugly message like:
```
"Internal error occured: IllegalArgumentException in ...."
```
In short, if you don't think your client programmer can explain your exception in a way that helps the user, then you should probably not be using a checked exception.

So those are my rules. Somewhat contrived, and there will doubtless be exceptions (please help me refine them if you will). But my main argument is that there are cases like FileNotFoundException where the checked exception is as important and useful a part of the API contract as the parameter types. So we should not dispense with it just because it is misused.

Sorry, didn't mean to make this so long and waffly. Let me finish with two suggestions:

A: API programmers: use checked exceptions sparingly to preserve their usefulness. When in doubt use an unchecked exception.

B: Client programmers: get in the habit of creating a wrapped exception (google it) early on in your development. JDK 1.4 and later provide a constructor in RuntimeException for this, but you can easily create your own too. Here's the constructor:

public RuntimeException(Throwable cause)

Then get in the habit of whenever you have to handle a checked exception and you're feeling lazy (or you think the API programmer was overzealous in using the checked exception in the first place), don't just swallow the exception, wrap it and rethrow it.

try {
  overzealousAPI(thisArgumentWontWork);
}
catch (OverzealousCheckedException exception) {
  throw new RuntimeException(exception);  
}

Put this in one of your IDE's little code templates and use it when you're feeling lazy. This way if you really need to handle the checked exception you'll be forced to come back and deal with it after seeing the problem at runtime. Because, believe me (and Anders Hejlsberg), you're never going to come back to that TODO in your

catch (Exception e) { /* TODO deal with this at some point (yeah right) */}

Best Answer

Related Solutions

Java – Why is using a wild card with a Java import statement bad

Java – The case against checked exceptions

Related Topic