I'm a naughty programmer, and until now I haven't handled errors properly (e.g. just catching a java.lang.Exception, printing a debug message, and moving on). When I do "handle" them, it's simply been to shut the compiler up.
I've recently learned the error (haha) of my ways, and would like to start doing it right. So I'm researching it here and other places (via Google searches).
Suppose I have a block of code which does the following:
...
x.method1(); // throws ExceptionTypeA
...
y.method2(); // throws ExceptionTypeB
...
z.method3(); // throws ExceptionTypeC
...
x.method4(); // throws ExceptionTypeA (again)
...
From what I've gathered, the proper way to handle this is:
try {
...
x.method1(); // throws ExceptionTypeA
...
y.method2(); // throws ExceptionTypeB
...
z.method3(); // throws ExceptionTypeC
...
x.method4(); // throws ExceptionTypeA (again)
...
} catch (ExceptionTypeA e) {
// do something about condition A
} catch (ExceptionTypeB e) {
// do something about condition B
} catch (ExceptionTypeC e) {
// do something about condition C
}
This seems pretty straightforward to me, but it seems to get messy when I have a long block of code which throws various errors throughout. I seem to wind up with just one giant try/catch around my whole method! The alternative seems to be:
try {
...
x.method1(); // throws ExceptionTypeA
...
} catch (ExceptionTypeA e) {
// do something about condition A
}
try {
...
y.method2(); // throws ExceptionTypeB
...
} catch (ExceptionTypeB e) {
// do something about condition A
}
try {
...
z.method3(); // throws ExceptionTypeC
...
} catch (ExceptionTypeC e) {
// do something about condition C
}
try {
...
x.method4(); // throws ExceptionTypeA
...
} catch (ExceptionTypeA e) {
// do something about condition A
}
This looks really nasty. In cases like this, I've considered doing something like the following:
private void doSomething() throws exceptionTypeA, exceptionTypeB, exceptionTypeC {
...
x.method1(); // throws ExceptionTypeA
...
y.method2(); // throws ExceptionTypeB
...
z.method3(); // throws ExceptionTypeC
...
x.method4(); // throws ExceptionTypeA (again)
...
}
public void doSomething_andHandleErrors() {
try {
this.toSomething();
} catch (ExceptionTypeA e) {
// do something about condition A
} catch (ExceptionTypeB e) {
// do something about condition B
} catch (ExceptionTypeC e) {
// do something about condition C
}
}
... and then just calling doSomething_andHandleErrors(); from outside. Is this a 'good' practice? Am I falling into some anti-pattern?
Thanks!
Source: Tips4all, CCNA FINAL EXAM
The main difference between your first and second example is how you handle the error itself. Is it transactional? In your first example, y.method2() will not run if x.method1() throws an exception. In your second example, it is possible depending on what the error handling does.
ReplyDeleteBoth of those are decent patterns, it's a matter of the business case required here. Do you want the exception to get passed to the caller so they can handle it? Do you want to do something else because of the error?
Also, do not forget the finally block. You'll want to make sure you use one if you're dealing with resource management (IO Streams, database connections, for example) so that you can do cleanup if necessary.
The point of handling exceptions is that you should be able to proceed further even when you face an exception. The first way and the third way are basically the same. If an exception occurs in the method1() you will directly exit the entire parent method without even an attempt at executing method2() and others.. The second method may seem cluttered at first but is actually the way in which it should be done.
ReplyDeleteEven better than that would be to handle the exceptions that you expect it to throw in the method itself and return a sort of default value which will allow further execution without breaking business logic or cause inconsistency.
EDIT:
Example of advantage when using 2 method:
Suppose you're making a text parser and expecting a date in the format DD-MM-YYYY. But while parsing you find that you get date in the format DD-MON-YYYY. These type of parsing exceptions can be handled and still allow further execution.
This really depends on where (on which level) you want to catch that exception.
ReplyDeleteA method that does throw an exception simply means that "I do not want to deal with this exception/problem, let anyone else catch" it. Clean code should come after this way of thinking; should I catch here or not...
In your last case if you throw again those exceptions, this would mean you will not need the objects x,y,z during error handling since they would most probably be out of scope.
I would prefer your first example if there's not more than a few lines between the method calls. Otherwise your second example would fit better. I've never seen the pattern in the last example, never in our codebase anyway.
ReplyDeleteIt's good that you are trying to create clean code.
ReplyDeleteIMHO, what you are doing is mildly excessive. Assuming you have to handle the exceptions, all you are doing is creating another method call. You still need a try/catch block. I would just do what you termed "the proper way" to handle it.
If you don't need to handle the exceptions and they represent failures from which you cannot recover, you could create Runtime exceptions, which will stop your program (assuming you don't catch them).
The last block of code you have looks good, apart from one thing. It's better if the exceptions extend from RuntimeException. That way you don't have to report what exceptions doSomething() throws.
ReplyDeleteKeeping the error handling separated from rest of your code is generally good practice. It keeps the other code clean.
both your first and third example seem to me like the best way to handle these kinds of situations, and are the way i usually go about handling the sometimes enourmous amount of exceptions my code can throw. Personally, i prefer the third option, its much more organized and concise, and doesn't fall into any anti-pattern that i know of. The second option is just ugly, and should be avoided, has it is just a big waste of space since you don't need such a big amount of try clauses do get things done.
ReplyDeleteIf there are some exceptions which are thrown only once by only a single block of code and if the place of exception handling does not affect the business logic, I prefer to handle them on the spot.
ReplyDeleteBut if the same exception can be thrown from multiple places, I like to handle it at the end, like your proper way.
Adding an extra method call looks a bit clumsy to me, because you are not avoiding the problem, all you are doing is placing it somewhere else.
One important thing missing here is the finally block, which I think is necessary irrespective of the exception handling style.
Of course, this is a personal choice, there can be no right or wrong answers, I guess.
Usually there aren't too many different things to do in a catch block, so you should be aiming to have one block for one behaviour.
ReplyDeleteSo if all you're doing is log and rethrow the exception, you should go for the first option. It's very rare that you have to handle each possible exception throw from multiple method calls separately, and choosing option two when not really needed greatly reduces readability, especially if you're assigning values to local variables too, which you have to declare and initialise outside the try block.
boolean success = false;
try {
success = doStuff();
} catch( ... ) {
...
}
This is quite a horrible code pattern and I try to avoid it if possible. The way to do it is to realise that stacking your catch blocks (option two) only makes sense if those catch blocks terminate normally (i.e. no exception is thrown from them). But in that case you can move the entire block into the method being called.
private boolean doStuff() {
try {
...do stuff...
return true;
} catch( SomeException ex ) {
...fidget around...
return false;
}
}
And you just call it like this:
boolean success = doStuff();
As an aside, Java 7 helps you with exception handling a lot, you can catch multiple exceptions in one catch block, or catch and rethrow neatly. It also helps you to do away with catch blocks altogether for things like closing connections. If there's no other factor holding you back, I would consider switiching to it.
The point of throwing an exception is to inform your caller that you were unable to complete the requested action, the point of catching an exception is to take the appropiate action in response to that failure. If your action doesn't differ based on the type of exception, then catching a specific type is useless.
ReplyDeleteAs the caller of a caught exception, your job is either to recover from the failure (try alternative methods, use defaults, whaever), or to manage the failure (cleanup, logging).
If the type of exception is helpful in doing either, then get the type, if you aren't doing either, let it bubble up to one of your callers that can.