tags:

views:

257

answers:

7

Suppose I have a MyException class that subclasses Exception. I'm using this class to include contextual information when errors occur in my code.

I typically use it to wrap one of the "standard" exception classes. For example, if an error happens during input validation, I will do something like

if (invalidInput())
  throw new MyException(new IllegalArgumentException(), arg1, arg2, ...);

But my IDE (Intellij IDEA) warns me that instantiating an unchecked exception (IllegalArgumentException in this example) without throwing it is bad, but doesn't tell me why.

So how sinful is it to instantiate an exception without throwing it? To which circle of hell will I go?

+2  A: 

You're not going to hell from instantiating an exception the way you're doing it. I'm sure the warning is there to make sure you use your exceptions as exceptions and not just any object.

However, you're going to hell for having MyException not subclassing RuntimeException ;)

Buhb
+8  A: 

You might be better off throwing an instance of IllegalArgumentException, in this case that's what it's for:

if (invalidInput())
         new IllegalArgumentException("Invalid argument " + x + ", expected ...");

Or otherwise extending, IllegalArgumentException instead of Exception, it if you want to enhance it with custom properties.

public class MyIllegalArgumentException extends IllegalArgumentException {  
    public MyIllegalArgumentException(Object arg...) {    ....  } 
}

Both cases provide a leaner, more meaningful, class model.

Update: given your comment about wanting to supply contextual info with the thrown exception - you can do this by supplying your custom exception object as the Throwable argument to the standard exceptions contructor i.e. flip it round so: instead of wrapping the relevant standard exception in your exception, you should wrap your exception in the relevant standard exception.

if (invalidInput())
         new IllegalArgumentException("Invalid argument " + x + ", expected ...", new MyContextException(a,b,c));

(where a,b & c are the various bits of context you want to transmit). This way you (re)use a meaningful & appropriate, exception at all points in the code, but you transmit the contextual information that you may want to use further up the stack when handling/logging the exception.

Joel
@Zed - just saw your comment about wanting to wrap all exceptions. Bare in mind that if you do this you will make exception handling harder and the code less readable. I updated my answer with an alternate suggestion that might better suit your needs.
Joel
+3  A: 

This isn't bad at all.

Like most warnings, they're there to indicate situations that are less likely to legitimately occur, than they are to be someone invoking them by mistake. Another recent example on SO was the warnings about synchronizing on a local variable; it's not often you want to do that and it's easy to mess up concurrency by doing it inadvertently.

IntelliJ is just warning you that exceptions are usually created to be thrown immediately, so if you aren't doing this it flags that perhaps your code is behaving unexpectedly. In your case, you're doing exactly what's right so feel free to ignore the warning. (In fact in my project the structure means I often create exceptions without throwing them for various reasons, so I downgraded the warning to an "info" level in IntelliJ's config).

Andrzej Doyle
A: 

Would it not better to pass a string of the type of error instead of the exception object? Why do you need the exception object?

When the MYException object is caught you could create an exception of type IllegalArgumentException and throw that if needs be.

(This should be a comment however I can't leave one yet).

Gordon
+1  A: 

It's a strange construct, but there is a solution to trick the compiler.
Rewrite your invalidInput method to throw the IAE

    private void checkInput() throws IllegalArgumentException {
        if (...)
            throw new IllegalArgumentException();

and:

    try {
        checkInput();
    } catch (IllegalArgumentException ex) {
        throw new MyException(ex, arg1, arg2, ...);
    }
Carlos Heuberger
A: 

IntelliJ does tell you why. You just have to read the description for the inspection.

This inspection reports any instances of Throwable instantiation, where the created Throwable is never actually thrown. Most often this is the result of a simple mistake.

Peter Lawrey
A: 

Can I ask please, those IntelliJ warriors out there (I'm a strict newbie -- started using it two days ago and I'm absolutely head-over-heels in love).....which particular inspection setting is this one controlled by?

I don't do exactly what the OP does/did, but I am triggering this inspection warning a lot in one of my files and I love the IntelliJ inspections so I don't want to get in the habit of ignoring them, which I will if this one continues to do its thing.

I've done a search in the inspection settings for "exception", and found nothing that relates exactly, so I turned them all off just to see, and it's still doing it (even after a rebuild...BTW when are inspections redone? at save? at rebuild? ???), so I would really like some help finding how to make this one into an info/typo level -- which I can then ignore.

Using the free version, if that makes any difference

TIA

skiaddict1
It's under Probable bugs -> Throwable instance not thrown
lindelof