views:

195

answers:

6

Hello,

I've seen this topic arise sometimes in the past but even after Googling about it, I still can't figure out what would be a good and elegant way to deal with it, so here it goes.

Say I have some code that throws various exceptions...

try {
  /* some code that throws these exceptions */
} catch (NoSuchAuthorityCodeException e) {
    throw new MyAPIException("Something went wrong", e);
} catch (FactoryException e) {
    throw new MyAPIException("Something went wrong", e);
} catch (MismatchedDimensionException e) {
    throw new MyAPIException("Something went wrong", e);
} catch (TransformException e) {
    throw new MyAPIException("Something went wrong", e);
}

... and as we can see, I just wrap these exceptions and throw a new one saying that something went wrong within my API.

This seems to me a overly repetitive code, so one would simply catch an Exception type and handle wrap it and throw a new one.

try {
  /* some code that throws these exceptions */
} catch (Exception e) {
    throw new MyAPIException("Something went wrong", e);
}

In this case it stays way more simpler, but the thing is we'd be catch also every RuntimeException. Given this, we could catch-re-throw the RuntimeException so we could avoid this.

try {
  /* some code that throws some exceptions */
} catch (RuntimeException e) {
    throw e;
} catch (Exception e) {
    throw new MyAPIException("Something went wrong", e);
}

Its a bit bulky but it does the trick. Now theres another little issue about the catch(Exception e), that if my inner API throws another MyAPIException it would also be caught, wrapped and throwed within another MyAPIException. In this particular case we could also catch MyAPIException and re-throw it.

try {
  /* some code that throws some exceptions */
} catch (RuntimeException e) {
    throw e;
} catch (MyAPIException e) {
    throw e;
} catch (Exception e) {
    throw new MyAPIException("Something went wrong", e);
}

Well, its getting messy again, but in this case we prevent wrapping MyAPIException and simply re-throw it. But, theres also another issue with the catch (Exception e) block that is if the inner API changes and start throwing another kind of exception (some other than these 4 mentioned above), the compiler wouldn't say anything about it and we wouldn't have a clue. Not that this would be a major problem, since probably I would treat it in the same manner.

With this scenario, I think the question is, which one is better and are there better options?

+2  A: 

Have more than one MyApiException. If you have every exception as MyApiException, it becomes a bit difficult for you and for others too to read your code. Naming it properly would also be crucial. Plus you do not always want to catch and rethrow it. If thats the case, simply declare a throws in method signature.

Plus you cannot fix on either of a simple catch or multiple catches. Its more of a judgmental decision IMO as there are exceptions which are fatal (as in the entire program has to halt) to more easy-to-handle types.

I do not see anything wrong at all when you have a bunch of throws clauses in your API. It is a much neater implementation. So what if the caller has to handle the exceptions that you've defined. If a method in your API may throw an exception and something has to be done about it, then the caller should have to handle it. Anyways, your documentation of that particular exception has to be clear in order not to make the caller confused.

One more factor that decides this is the complexity of the API. Sometime back I had written an API which was moderately complex and I had to deal with the same issues like the one that you've presented. I had written few custom exceptions for few methods. I am sure you will not end up throwing exceptions for all the public method that the API exposes, but there are places which they are inevitable.

At last, if you feel having too many custom exceptions a bitter choice, you can have one single exception with clearly documented error codes. This way the caller can handle the exception and deal with the error codes as he sees fit.

Bragboy
The thing is if I simply start throwing all other exceptions, someone else would have to deal with all those exceptions and I don't think its very nice to have a method that throws lots types of exceptions. Also if I had to have one kind of exception to every wrapped exception, that would be simply silly... why not simply throw it? (and now we have a recursion)
Pablo Cabrera
@Pablo Cabrera : See my edited reply
Bragboy
Well in this case, I just wanna show it to the user of this API that something went wrong and it could not be completed so (s)he can deal with this in a simple catch block (catch (MyAPIException e)) instead of catching multiple kinds of exceptions or having to deal with this same catching issue presented here. In case the user wanted to know the reason why it failed, (s)he still can look at the cause inside the exception. I think something like this would be a way neater API, but as you said, it might be a judgmental decision. Any thoughts?
Pablo Cabrera
As you pointed out, we could also have lots of specific exceptions for each case, but IMO it would be wise to make all those exceptions extend from a single Exception within you API. Like this, the user could handle each case (specific catches) or one catches all (catch main API exception) depending on his/her needs.
Pablo Cabrera
+2  A: 

In JAVA 7 you will be able to do something like

try {
  /* some code that throws these exceptions */
} catch (NoSuchAuthorityCodeException , FactoryException, MismatchedDimensionException , TransformException e) {
    throw new MyAPIException("Something went wrong", e);
}

So perhaps the best answer is to chill out for a while and then upgrade to JAVA 7 when it becomes available.

emory
Yeah, I heard about it but the thing is, at the company that I work we still use Java5(!!!), and I'd like to do something elegant within Java5 or Java6 for the matter
Pablo Cabrera
+5  A: 

Since you are stuck with Java5 and you use a proprietary exception, why not put all that exception logic into the exception class.

Useage

try
{
     // some code that might throw one of several exceptions
}
catch ( Exception cause )
{
     MyAPIException . handle ( cause ) ;
}

MyAPIException contains the logic

class MyAPIException extends Exception
{
    private MyAPIException ( String message , Throwable cause ) { super ( message , cause ) ; }

    private static void myAPIException ( Exception cause ) throws MyAPIException
    {
         throw new MyAPIException ( "Something Went Wrong" , cause ) ;
    }

    public static void handle ( Exception e ) throws MyAPIException
    {
          try
          {
               throw ( e ) ;
          }
          catch ( RuntimeException cause )
          {
                throw cause ;
          }
          catch ( MyAPIException cause )
          {
                 throw cause ;
          }
          catch ( NoSuchAuthorityCodeException cause )  // repeat for other exceptions
          {
                 myAPIException ( cause ) ;
          }
          catch ( Exception cause ) // this should not happen
          {
                  assert false ; // or log it or throw a RuntimeException ... somehow signal that programming logic has failed
          }
    }
}
emory
I dont really wanna catch the RuntimeException and MyAPIException
Pablo Cabrera
it would be nice if you put a usage example.
João Portela
@Pablo Cabrera I added a usage example. It will throw the RuntimeException and MyAPIException. I put everything in s static method in the MyAPIException class. You can call this from your code with one simple try/catch.
emory
+1. I don't think this is the best solution, but it's a very interesting approach.
João Portela
Its not very flexible... What if I would throw this exception on another context (which I do)? Should I handle those other exceptions as well? Should I code a specific "handle" for each usage?
Pablo Cabrera
@Pablo Cabrera If you always handle exceptions the same way, then you can use one method (e.g. handle) to do it - eliminating duplicate code. Modify the given handle method by adding catch blocks for all the Exceptions you wish to support. If you do not always handle exceptions the same way, this is less useful.
emory
@emory But then I would have the same number of "catches" as I had in the first example...
Pablo Cabrera
+1  A: 

It all depends what you you want to do. If all you want to do is throw new MyException("Something went wrong", e); then one catch all will do.

fastcodejava
As I stated before, I don't wanna catch RuntimeException or MyAPIException... RTFQ
Pablo Cabrera
+1  A: 

OK guys, here is what I came up with... It's not that elegant but I think is a bit better than having various catches.

First, we have an abstract class called ExceptionHandler.

package my.api.pack;

import java.lang.reflect.ParameterizedType;
import java.util.LinkedHashSet;
import java.util.Set;

public abstract class ExceptionHandler<E extends Exception> {

    Set<Class<? extends Exception>> exceptionClasses = new LinkedHashSet<Class<? extends Exception>>();

    protected abstract void handler(Throwable t) throws E;

    public ExceptionHandler<E> catches(Class<? extends Exception> clazz) {
        exceptionClasses.add(clazz);
        return this;
    }

    @SuppressWarnings("unchecked")
    private Class<E> getGenericsClass() {
        ParameterizedType parameterizedType = (ParameterizedType) getClass().getGenericSuperclass();
        return (Class<E>) parameterizedType.getActualTypeArguments()[0];
    }

    @SuppressWarnings("unchecked")
    public final void handle(Throwable t) throws E, UnhandledException {
        if (getGenericsClass().isInstance(t)) {
            throw (E) t;
        }

        for (Class<? extends Exception> clazz : exceptionClasses) {
            if (clazz.isInstance(t)) {
                handler(t);
                return;
            }
        }

        throw new UnhandledException("Unhandled exception", t);
    }

}

Along with it, we have this simple runtime exception called UnhandledException

package my.api.pack;

public class UnhandledException extends RuntimeException {

    private static final long serialVersionUID = -3187734714363068446L;

    public UnhandledException(String message, Throwable cause) {
        super(message, cause);
    }


}

With it we can use these to handle exceptions like this...

try {
  /* some code that throws these exceptions */
} catch (Exception e) {
    new ExceptionHandler<MyAPIException>() {
        @Override
        protected void handler(Throwable t) throws MyAPIException {
            throw new MyAPIException("Something went wrong", t);
        }
    }.
        catches(MismatchedDimensionException.class).
        catches(NoSuchAuthorityCodeException.class).
        catches(FactoryException.class).
        catches(TransformException.class).
        handle(e);
    return null;
}

What you guys think?

Pablo Cabrera
emory
(5) Replace UnhandledException with "assert false: "Unhandled Exception;" I don't like UnhandledException. If the ExceptionHandler is instantiated correctly, there should be no UnhandledExceptions. So the presence of UnhandledException indicates a programming error.
emory
Now, I think I understand better what u want. So I have another answer below.
emory
A: 

Below is another approach and a usage example. The MyAPIException class has a handle method. The handle method will handle any Exception.

  1. If the Exception is a Runnable or a MyAPIException, handle will throw it without wrapping it.
  2. If not, the handle method will test whether assertions are enabled. If assertions are enabled, the handle method will test to see if the exception is assignable from one of the supported exception types, throwing an AssertionError if it is not assignable. (If assertions are not enabled, the handle method ignores the supported exception types parameter.)
  3. Finally - if it reaches this point - the handle method will wrap the exception in a MyAPIException and throw it.

When you test your code, run it with assertions enabled. In production, run it with assertions disabled.

<soapbox> If you use this technique, you will have to test for errors that the compiler would have otherwise caught.</soapbox>

class MyAPIException extends Exception
{
    private static final long serialVersionUID = 0 ;

    MyAPIException ( Throwable cause )
    {
        super ( cause ) ;
    }

    static void handle ( Exception cause , Class < ? > ... supportedExceptionTypes ) throws MyAPIException
    {
        try
        {
        throw ( cause ) ;
        }
        catch ( RuntimeException e )
        {
        throw ( e ) ;
        }
        catch ( MyAPIException e )
        {
        throw ( e ) ;
        }
        catch ( Exception e )
        {
        search :
        try
            {
            assert false ;
            }
        catch ( AssertionError a )
            {
            for ( Class < ? > c : supportedExceptionTypes )
                {
                if ( c . isAssignableFrom ( e . getClass ( ) ) )
                    {
                    break search ;
                    }
                }
            assert false : e + " is not one of the supportedExceptionTypes : " + supportedExceptionTypes ;
            }
        MyAPIException error = new MyAPIException ( e ) ;
        throw ( error ) ;
        }
    }
}

This is an example of usage. You can run it with assertions enabled/disabled and with parameters 1,2,3,4 to see how it handles different situations.

class Usage
{
    public static void main ( String [ ] args ) throws MyAPIException
    {
    try
        {
        switch ( Integer . parseInt ( args [ 0 ] ) )
            {
            case 1 :
            throw new RuntimeException ( ) ;
            case 2 :
            throw new SQLException ( ) ;
            case 3 :
            throw new MyAPIException ( null ) ;
            case 4 :
            throw new IOException ( ) ;
            }
        }
    catch ( Exception cause )
        {
        System . out . println ( cause . getMessage ( ) ) ;
        System . out . println ( cause . getCause ( ) ) ;
        MyAPIException . handle ( cause , IOException . class ) ;
        }
    }
}
emory