{ by david linsin }

February 18, 2009

An Aspect is Part of Your Implementation

When implementing parts of your application with AOP, you are magically applying additional behavior to your code. Although technically cool and in terms of reusability great, it might lead to a maintenance nightmare.

Think of an API, let's call it CustomerService. It defines CRUD methods to load and save Customer instances from some backend service. It might define methods like the following:

/**
* blah
*
* @throws CustomerServiceException with following causes:
* - IllegalArgumentException if anything is wrong with the passed Customer instance
* - NullPointerException if anything is wrong with a collaborator
* - FancyPersistenceException if anything is wrong your persistence service
*/
public Customer save(Customer argToSave) throws CustomerServiceException;


Note the standard Javadocs, which define precisely the causes of a CustomerServiceException, which the client of this API can expect. These defined causes as well as the CustomerServiceException are part of the contract defined by the interface. Clients can program against those causes e.g. display different error messages or inform another system.

An implementation of this interface might look as the following:

@Override
public Customer save(Customer argToSave) throws CustomerServiceException {
try {
// do some fancy stuff
} catch (IllegalArgumentException e) {

// do some logging here
throw new CustomerServiceException(e);

} catch (NullPointerException e) {

// this is programming error
throw new CustomerServiceException(e, "Dude where's your head?");

} catch (FancyPersistenceException e) {

// you might want to do some transaction handling stuff here
throw new CustomerServiceException(e);

} catch (Exception e) {
throw new CustomerServiceException();
}
}


This is pretty ugly isn't it? Especially because of the verbose Exception handling. However it's complying with the contract and a valid implementation.

In my current project we followed this pattern in every service class throughout the application. This works fine and provides a lot of consistency for clients of those APIs. However making a commitment like this requires not only a lot of discipline, when implementing those contracts, but also a lot of test code. You have to make sure that each implementation complies with the contracts promised in its interface.

A new member of the team immediately pointed out the drawbacks of our approach: a lot of test code to maintain, a lot of verbose Exception handling polluting the implementation methods and no centralized way to manage the same recurring code. His idea was to use AOP. You can refactor the Exception handling into an Aspect and thus get rid of all the drawbacks mentioned before.

The implementation of the CustomerService interface boils down to the following:


@Override
public Customer save(Customer argToSave) throws CustomerServiceException {
// do some fancy stuff
}


For a moment this might look like a perfect solution. The ugly technical Exception handling is gone and we are left with the code that actually gets the job done. That makes reading the code so much easier and don't think I'm the only fan of that.

Despite of all the drawbacks we go rid of, we silently introduced new ones. The fact that the implementation doesn't contain any hard reference to those Exceptions we claim to throw in the interface, makes understanding the implementation of the contract a lot harder. Not to mention the code for the Exception handling itself. Another drawback is refactoring the interface. Imagine someone unfamiliar with the code has to change one of the Exception causes. It is merely impossible to understand what's going on.

You have to consider the Aspect containing the logic to handle the Exception as of your implementation. Without it your implementation doesn't comply with the contract.

So what to do now? We have a great solution, which relieves us from all the drawbacks but brings along another bag of drawbacks. Should we gauge which drawbacks are easier to deal with? I remembered a solution I read in Applying UML and Patterns, which suggests a Singleton based centralized error handling. So I came up with the following solution:


public final class ExceptionHandler {
// private constructor
public static final <T extends BaseException> transform(Exception argCause, Class<T> argType) {
try {
if (argType.isAssignableFrom(argCause.getClass())) {
return (T) argCause;
} else if (argCause instanceof NullPointerException) {
return argType.getConstructor(Throwable.class, String.class).newInstance(argCause,"Dude where's your head?");
} else if (argCause instanceof FancyPersistenceException) {
return argType.getConstructor(Throwable.class).newInstance(argCause);
} else {
return argType.getConstructor(Throwable.class, String.class).newInstance(argCause,"Unkown!");
}
} catch (Exception e) {/** do some reflection error handling **/}
}
}


The implementation of the CustomeService interface using the centralized ExceptionHandler looks like this:


@Override
public Customer save(Customer argToSave) throws CustomerServiceException {
try {
// do some fancy stuff
} catch (Exception e) {
throw ExceptionHandler.transform(e, CustomerServiceException.class);
}
}


I believe this is a great trade-off. You have a single point in your code responsible for handling Exceptions. It's easy to test in isolation, which means the test code for your services decrease. The overhead in your service methods is minimal. You have to implement a try-catch block, but I think that's reasonable considering the benefits of this approach.

However, I think the biggest advantage of this solution is the fact that it works without any framework magic whatsoever. It's just plain old Java!

NOTE: Over the next few weeks, I'm moving my feed from feedburner back to blogspot so please subscribe to http://dlinsin.blogspot.com/feeds/posts/default/ instead!

4 comments:

Ollie said...

Hey David,

IMHO your sample lacks plausibility a little. RuntimeExceptions should be caught only in rare cases (IncorrectResultSizeDataAccessException e.g.) but rather catched in a single place (maybe an Aspect? ;). So you might guess, I am not a big fan of declaring RuntimeExceptions like IllegalArgumentException in JavaDoc. If you do so, where do you draw the line? You actually would have to annotate each and every method with all RuntimeException types (exeggaration serves clarity in this case ;). To me this sounds weird as you tend to push the API client to catch these Exceptions although they do not appear in the method signature. I don't say you should never catch RuntimeExceptions, but being pushed to do so feels uncomfortable to me.

If you feel a client should (and schould be able to) handle an Exception make it checked. That's actually what they were made for. Catching NPEs seems weird to me. Code throwing NPEs is wrecked. Period. Even IllegalArgumentException seems strange to me. If you have a need to catch IllegalArgumentException you actually should catch at least a custom RuntimeException instead of the IAE.

Nevertheless I agree with your approach to write an dedicated ExceptionHandler. In a customer project we also have an AOP solution and happily live with it. So I guess it depends... :)

Regards,
Ollie

david said...

hey ollie,

I guess my examples were a little unclear, but I'm not declaring that the API declares RuntimeExceptions of any type. The API declares a single Exception which wraps all other Exceptions. So the Client only handles one type of Exception.

The implementation of the CustomerService though has to deal with all sorts of Exceptions. They need to be handled and thus having a consistent pattern and place to catch those Exceptions is important. Doing it in an Aspect is IMHO not the best solution, because it is part of a contract that is not visible. You have to know that Exceptions are being handled by an Aspect otherwise you are misled.

But as always it depends, I guess...

Anonymous said...

most of the new 'technologies' stand or fall with the support of IDE's and tools. if you have support for aspects in your IDE, the lines where aspects are woven will be marked by your IDE. if the ide does a got job, you can easily navigate to the aspects. one issue solved

david said...

You definitely got a point there. A good IDE will solve a lot of problems with technical aspects.

However it doesn't change the fact that you tie your implementation to the Aspect to comply with the contract defined in your interface...maybe there is no "right" solution to this and it boils down to a matter of taste.


com_channels

  • mail(dlinsin@gmail.com)
  • jabber(dlinsin@gmail.com)
  • skype(dlinsin)

recent_postings

loading...