views:

138

answers:

5

I am currently working on the maintenance of a piece of code that is a little bit "Exception Happy." Basically, ever method or anything throws Exception. I'm going to work to take care of that, but, in the meantime, I am wondering what is the best way to handle individual exceptions within a smaller block of code, such as a method. Is it better to do something like this:

public void aMethod()
  try {
    //Lots of code in here.  Many lines.
  } catch(Exception e) {
    // TODO - Handle All Exceptions, but fairly generically
  }
}

Or something like this:

public void bMethod() {
  try {
    // One line of code.
  } catch(Exception e) {
    // TODO - Handle a specific Exception (may even involve rethrowing it with more information)
  }

  // More code.

  try {
    // Another line of code.
  } catch(Exception e) {
    // TODO - Handle another specific exception.
  }
}

I realize this is a pretty basic question, but after looking at hundreds of methods with Exceptions coming out of every one, I'm starting to wonder how best to handle all of them and what a best practice may be here.

+5  A: 

First off, you should only put code in try/catch blocks that is exception worthy. For instance, having an unexpected value is not necessarily an exception, but trying to read from a file that doesn't exist is.

To answer your main question, you should put the exceptionable code in the same try {} block and catch specific questions in order of granularity in multiple catch blocks after the main try.

//Regular code that doesn't need to be covered by a try/catch block

try {

  //critical code only

} catch (NullPointerException npe) {
  //Code
} catch (RandomException re) {
  //code
} catch (Exception e) {
  //code
}
Robert Greiner
Thanks for your answer! Also, I do understand exception-worthiness. Unfortunately, the original author literally throws java.lang.Exception everywhere, which means I have to handle every exception (or my method must throw it), so, at this point, I don't get an option. I will work towards a much more coherent solution. Starting with baby steps first (which this question is part of).
JasCav
@Jason, no problem. It's nice to see people putting in the extra effort to improve existing code when they don't have to. Keep up the good work!
Robert Greiner
+1  A: 

your bMethod isn't very useful illustration. Try to reprase it. Anyway, you have two options:

  1. catch exceptions and log them.
  2. catch exceptions and throw new RuntimeException(ex) (rethrow a runtime exception, setting the original as a cause)

Also, you will need to differentiate between exceptions. If your system has many custom exceptions, they are probably defined for a reason, and specific behaviour is expected when one of them is thrown.

Bozho
"catch exceptions and throw new RuntimeException(ex)" so you propose to replace a strongly typed system with a weakly typed one?
rsp
he seems willing to get rid of all those exceptions. It vastly depends on the exceptions themselves, and how should the program flow continue.
Bozho
Yeah, I'm working on switching over to unchecked exceptions in many cases (where it makes sense to). Unfortunately, the system has no custom exceptions. While this is not always a bad thing, necessarily, it is in this case.
JasCav
I'm not a fan of rethrowing unchecked exceptions, either handle the exception or let it propagate upwards checked. If exceptions are used as a type of return value, it is a sign of bad design (imho.)
rsp
+2  A: 

The answer to your question is: it depends.

  • if the code in the try block is coherent where it makes no sense to proceed in the event of an error, the first approach is best
  • if the code is taking seperate steps that are relatively unrelated (parsing numbers for instance) and can be recovered without aborting the rest of the method the seconds appraoch makes sense

A general remark on the code you inherited; it sounds like exceptions are abused to pass state around, I would refactor so that exceptions are caught at the spot where they can be handled and introduce return values or attributes to handle the state.

rsp
A: 

If the try/catch block isn't adding useful information, just allow the method to throw the exception and have it handled where the caller can sensibly do something with the exception. This could cut down the number of try/catch significantly (by an order of magnitude).

BTW a trick for rethrowing any exception is.

try {
   // do something
} catch (Throwable t) {
   Thread.currentThread().stop(t); // rethrow any exception.
}
Peter Lawrey
A: 

In addition to the suggestions already made you may also want to consider extracting try/catch blocks from the "meat" of the function.

public void delete(Page page) {
    try {
        deletePageAndAllReferences(page);
    }
    catch (Exception e) {
        logError(e);
    }
}

private void deletePageAndAllReferences(Page page) throws Exception {
    deletePage(page);
    registry.deleteReference(page.name);
    configKeys.deleteKey(page.name.makeKey());
}

private void logError(Exception e) {
    logger.log(e.getMessage());
}

This lets you focus your attention on the function you are really interested in without the exception handling getting in your way.

codeelegance