Saturday, February 12, 2011

wrapped exception from a component

There is this style of exception system where a component throws component-specific exception. For example, all data access classes throw DataAccessException.

In this style, I often find myself having to catch and rethrow the component specific exception, because called methods are delcared as throws Exception:

try {
    int foo = foo();
    if (foo != expectedValue) {
        throw new ComponentException("bad result from foo(): " + foo);
    }
    bar();
}
catch (ComponentException e) { 
    throw e; 
}
catch (Exception e) { 
    throw new ComponentException(e); 
}

Do you find yourself doing the same? Do you find it ugly?

This question is not about validity of this style, but something within the constraints of this style.

  • That is ugly as hell. There isn't much more to comment on that ugly style, I think. If you already have all the code to handle different issues using the cause, that'd work. It's just that using

    try {
      componentCall();
    } catch (ComponentException e) {
      Throwable t = e.getCause();
      //Handle each possible cause
    }
    

    is less maintainable and more confusing than the other way, and the main point is that I see no advantage to using it.

    Given that you are restricted to using it, I'd at least try to avoid throwing the ComponentException both with and without a cause, that makes it more confusing than it needs to be, in your example I'd add a InvalidFooException and add it as a cause for the generic ComponentException.

    try {
        int foo = foo();
        if (foo != expectedValue) {
            throw new InvalidFooException("bad result from foo(): " + foo);
        }
        bar();
    }
    catch (Exception e) { 
        throw new ComponentException(e); 
    }
    

    getCause() is intended for chains of exceptions that are only causally instead of semantically related.

    If you need them to differentiate between different exceptions (for instance, a SQLExecutionException should be corrected by a different action than an AccessDeniedException) then your method is not applicable (because forcing the use of getCause() in each catch to see what needs to be done is awful and offers no benefit than just catching the right exception out front).

    If all the calling classes have to do is report an error and cancel, then the wrapping might be okay, but I wouldn't do it, as it adds little benefit and if you later need to differentiate will make you rewrite lots of stuff.

    What is useful is to create a hierarchy of exceptions for exception of the same type, in the lines of:

    If DataAccessException would be the root, then you could for example have DataSourceUnavailableException, InvalidDataSourceException, InvalidDataException and then you can decide to catch either parent only (if the action is the same) or to catch each exception separately.

    Why do you need this to behave this way, by the way?

    Hemal Pandya : You are commenting on the Style:-) Please assume I am constrained to follow it (ever been in that situation?). The question is about the first catch actually. It is not useless, without it, we get a ComponentException with another ComponentException as cause, which is not desired.
    Vinko Vrsalovic : How can throw new ComponentException("bad result from foo(): " + foo); throw a ComponentException with a ComponentException as a cause?
    Vinko Vrsalovic : I think I touched the constraints the style imposes on you, as well as mentioning I do find it ugly
    Vinko Vrsalovic : Oh, I got it.. you can't get rid of the catch(Exception e) {}
    Hemal Pandya : thanks, Vinko. I'll try the InvalidFooException, but looking for beauty in an inherently faulty style may be too much to ask for
  • It would maybe less ugly to do so (if the function foo() is the one declared to throw Exception) :

    int foo;
    try {
        foo = foo();
    }
    catch (Exception e) { 
        throw new ComponentException(e); 
    }
    if (foo != expectedValue) {
        throw new ComponentException("bad result from foo(): " + foo);
    }
    bar();
    
    Hemal Pandya : thank you for your response. The problem with this approach is that it becomes very verbose. In practice, foo call is preceded and succeeded by many other calls and this approach will require a separate try blocks for everytime ComponentException has to be thrown
    From Vinze

0 comments:

Post a Comment