CODEWITHSUNDEEP
javaexception
DaveM
DaveM

Reputation: 734

is chaining exceptions valid

During testing of a class I discovered that my class constructor could take an "illegal argument". The constructor was expecting a URI. Obviously if there is nothing suggesting that the passed argument is a URI, then it throws out an IllegalArgumentException.

My constructor originally tested the passed argument to see if it was a valid URI (by creating a file from it). I then modified this to try the file creation and catch the IllegalArgumentException. In the body of my catch block I simply do:

throw new URISyntaxException(passedArgument, message)

Is this a valid method for catching the possible exceptions that may be thrown, or should I be doing it some other way?

Also, as I found this issue while testing, am I justified in simply modifying the code to throw the exception I expect (and which is a more obvious error to present to the user, and put into the log)?

Edit 1: In response to comments, here is an example of my code:

public myClass (String fileName) throws URISyntaxException {
    try {
        fileToRead = new File(fileName);

        if ( !fileToRead.canRead() ) { //confirm we can read the passed file
            // if not, throw a URI error
            throw new URISyntaxException(fileName, 'bad filename passed, please check path and try again');
        }
    } catch ( IllegalArgumentException e ) {
        throw new URISyntaxException(fileName, 'bad filename passed, please check path and try again');
    }
}

In essence my question is, is it valid to throw a URI exception inside the catch block of the IllegalArgumentException? Is this a valid practice, or could I do this better?

Upvotes: 2

Views: 204

Answers (2)

Ilmari Karonen
Ilmari Karonen

Reputation: 50378

What you're doing isn't really exception chaining, unless you're actually wrapping the exception you caught inside that one you're throwing. Instead, you're simply detecting the violation of a precondition by attempting an operation which you know will throw a certain exception if the precondition is violated, and then reporting the violation by throwing another exception.

The only potential problem I see with that is whether you really know for sure that the operation you're attempting will only throw the exception you're catching in that particular situation. If there might be more than one reason why this operation could throw an IllegalArgumentException, but you're assuming that the cause will always be a specific one, then the exception you throw to report the issue might end up being misleading (e.g. reporting a valid URI as invalid, just because some other argument was invalid).

Basically, whenever you're catching a generic exception and throwing a more specific one instead, there's a risk that you might be mistaken about the cause of the generic exception. But if you're sure that no such risk exists — for example, if the code you're calling clearly documents which exceptions it may throw under what conditions — then it should be fine.

Edit after actual code was added to the question:

Mostly unrelated to the general principles discussed above, I see several specific issues with your code.

  1. First, if the File class your code uses is indeed java.io.File, and you're passing the filename to the constructor as a String rather than as a URI object, then the constructor you're calling isn't documented to throw an IllegalArgumentException in any case, so trying to catch one is pointless. The only File constructor that throws those is this one.

  2. Second, the fact that File.canRead() returns false doesn't necessarily mean that the filename is invalid — it might be perfectly valid, and the file might even exist, but your program might not have the necessary permissions to read it.

  3. Finally, as others have pointed out, I'm not sure what the "URIException" you're throwing actually is, unless it's a custom class you've written (there is a javax.print.URIException, but that's an interface, not a class; the only other one I could find is this one from the Apache HTTP Client), but the name, at least, seems completely inappropriate for an exception which you're using to report an invalid (or unreadable) file name. You really should use some other exception for that.

Upvotes: 4

Aleksandr Dubinsky
Aleksandr Dubinsky

Reputation: 23535

Yes, it's both valid and a best practice, but you should be throwing a custom exception or an IllegalArgumentException saying the constructor parameter is wrong, not a UriException

Upvotes: 5

Related Questions