26

Note on the question: this is not a duplicate, Efficient try / catch block usage? was asked after this one. The other question is the duplicate.

I was wondering what was the best way to use try/catch. Is it better to limit the content of the try block to the minimum or to put everything in it?

Let me explain myself with an example:

Code 1:

try {
  thisThrowsAnException();
}
catch (Exception e) {
  e.printStackTrace();
}
thisDoesnt();

Code 2:

try {
  thisThrowsAnException();
  thisDoesnt();
}
catch (Exception e) {
  e.printStackTrace();
}

Assuming that thisThrowsAnException()... well... can throw an exception and thisDoesnt()... I'm sure you got it.

I know the difference between the two examples: in case the exception is caught, thisDoesnt() will be called in the first case, not in the second. As it doesn't matter for me, because the throwing of that exception would mean the application should stop, why would one use a version better than the other?

Community
  • 1
SteeveDroz
  • 665
  • 3
  • 8
  • 15
  • @Michael Thanks for the tag, it's sometimes difficult to identify them. I tried `[try-catch]` but it didn't exist ;-) – SteeveDroz Jun 09 '11 at 08:43
  • @Otarus - no problem :) – Michael Jun 09 '11 at 09:25
  • I don't like the catch block. Why catch the exception only to print a stack trace, and then continue execution like nothing happened? That just fills your code with catch blocks. Let the exception propagate to the outer loop where something sensible can be done. – kevin cline Jun 09 '11 at 12:38
  • @kevin Tthis example is not real... I usually put more important stuff in a catch block. – SteeveDroz Jun 09 '11 at 12:48
  • 2
    @Oltarus: I thought that might be the case. I try to limit catch blocks to an absolute minimum, only handling the ones I can truly recover from. Otherwise I just let them propagate and catch them all in one place. – kevin cline Jun 09 '11 at 13:21
  • @kevin Ok, I never heard of that way of doing! Interresting. Thanks for sharing! – SteeveDroz Jun 09 '11 at 13:39

8 Answers8

48

In my opinion you should put everything in the block that is dependent on the part that throws the exception. So if in your second example:

try {
  thisThrowsAnException();
  thisDoesnt();
}
catch (Exception e) {
  e.printStackTrace();
}

If thisDoesnt(); is dependent of a successful execution of thisThrowsAnException() it should be included.

Does it make sense to run it if thisThrowsAnException() fails?

Michael
  • 634
  • 1
  • 6
  • 6
17

It is important to note that by catching an exception you are undertaking the responsibility of recovering from it. The scope of the try block should depend on how that recovery is to be performed. For example, look at these two code snippets:

Snippet 1:

for (Object object : objects) {
    try {
        performTaskOnObject(object);
    } catch (Exception e) {
        log.error("Failed to perform task on object", e);
    }
}

Snippet 2:

try {
    for (Object object : objects) {
        performTaskOnObject(object);
    }
} catch (Exception e) {
    log.error("Failed to perform task on objects", e);
}

By simply changing the scope of the try block, these two snippets present two very different recovery strategies. Snippet 1 allows the task to be attempted for the remaining Objects even after one or more failures. Snippet 2 does not allow the task to be attempted for the remaining Objects after a failure.

Your recovery strategy should be your guide to scoping your try block.

Gyan aka Gary Buyn
  • 2,775
  • 19
  • 17
12

Like Michael already mentioned, it's not only a question of coding style or best practice, you have to keep the algorithm in mind.

The advantage of code 1 is maintainablity. The try/catch wraps only that one line of code that may throw the exception. So if we see a try/catch block, it's immediatly cleary, which operation can throw it. This is quite valuable if we have a series of operations that throw various IO-Exceptions. And if we remove a statement or refactor some part of the code into a separate method, then we don't lose our exception handlers or leave unnecessary try/catch handlers (of catched runtime exception).

The advantage of code 2 is readabilty. To many try/catch blocks really clutter a method and it becomes extremely difficult to see the real algorithm behind all those try/catch statements and handlers.

In my own code I prefer version 2, the version that has a higher chance to lead to clean code. Even though, I have to pay more attention during refactoring.

Trivia - what you shouldn't put in a catch block is a e.printStackTrace(); statement ;) I'd really recommend to add log4j (or something similiar) and change the IDE's templates to initialize the catch block with log.debug(e)

Community
  • 1
Andreas Dolk
  • 926
  • 6
  • 10
  • 2
    +1 for suggesting the use of logging framework such as log4j instead of printing the error stacktrace. Though you should use `log.error(…)` or `log.warn(…)` instead depending on the severity of the exception. – Spoike Jun 09 '11 at 08:53
  • @Spoike - thought about the log level and decided for `DEBUG` as a *default for the template*. We can/should change the log level and add a custom message if needed after the try/catch block has been autogenerated by the IDE. It was my personal vision, that if we don't implement a catch block actively, the message is more `DEBUG` then anything else. – Andreas Dolk Jun 09 '11 at 09:44
  • @Andreas_D: Thought about it too for a while before you commented and I agree, it'd be a good idea to do `log.debug` as default for templates. – Spoike Jun 09 '11 at 10:13
  • I don't like accepting only one single answer when I'm asking a question about opinions, but this one sums up the pro/con and gives a global view of the answer... – SteeveDroz Jun 10 '11 at 07:10
  • Regarding logging, I've been very happy with the [SLF4J](http://www.slf4j.org/) compatibility layer. – Darien Jun 10 '11 at 08:15
2

I would prefer the first version, so it becomes clear for the human reader which part of the program throws the exception and how it is handled. In a real world program, thisDoesnt() would be several lines of code, and by pure luck, one of them might throw exceptions too, but version 2 of the code would hide that fact, so the exception handling might do the wrong thing because it's meant to handle a failure of thisThrowsAnException();. And those several lines of thisDoesnt() seperate the exception handler from thisThrowsAnException();, making it harder to see the connection.

Therefore, my version of the code would look like that

boolean success=false;
try {
  thisThrowsAnException();
  success=true;
}
catch (Exception e) {
  e.printStackTrace();
}

if (success) {
  thisDoesnt();
}

or with an early exit in case of an error:

try {
  thisThrowsAnException();
}
catch (Exception e) {
  e.printStackTrace();
  return null;  // false, "", whatever; or rethrow the exception
}

thisDoesnt();
user281377
  • 28,352
  • 5
  • 75
  • 130
0

Note there is two ways to implement "the application should stop".

  1. Throwing an exception and let it bubble all the way up to a central error handler that do what you need to have done and then exits the program (or try again or whatever).

  2. Calling "System.exit(1)". This is rarely what you want, except in the simplest programs, and even rarer in e.g. a web application where a simple error could crash the web server. Also, resources are not released properly, etc.

I would strongly recommend learning how to use exceptions to indicate and recover from errors instead of using System.exit(). The finally clause is also very important for cleaning up resources.

In your question, if thisDoesnt() should be executed regardless whether thisThrowsAnException() fails or not, then it is code 1. If it only should be run if not failing, then code 2.

0

The two codes are doing different things in presence of exception. There is no preference with one or the other, use the one which does what is needed.

AProgrammer
  • 10,404
  • 1
  • 30
  • 45
-1

Its going to be a game-time decision for each one. If you have code that can throw exceptions that is not absolutely necessary for the program, then put it alone in the try block. That way in the catch block you can deal with where to go next.

A good design pattern for this is to have a helper method which can determine if an error is fatal (end the session, close the program, whatever) or not, and then rethrow or not based on the result of that check. Something like:

try{
    foo();
}
catch(Exception ex)
{
    bool rethrow = ExceptionHelper.Handle(ex);  //should return true if error is fatal, false if not
    if(rethrow) throw;
}
Phillip Schmidt
  • 119
  • 4
-1

In my opinion, there can be two conditions,one that we want to execute the program evenafter exception occurs and the next we don't. The later one is simple just log in the errow or give a proper definition that exception that has occured and run away.

In first case:

Lets make it simple to understand

We are writing a program to add list or numbers like (a1/x)+(a1/x)+(a3/x)+...

like this:

try {
  sum=(a1/x)+(a1/x)+(a3/x)+...;
}
catch (Exception e) {
  e.printStackTrace();
}
syso.(sum);

Now if somebody inserted x=0 then an exception occurs. Now do you want the program to end like this?

we would than want to write like this

try {
  sum=(a1/x)+(a1/x)+(a3/x)+...;
  syso.(sum);
}
catch (Exception e) {
  e.printStackTrace();//optional
  syso("Hey man! You cannot divideanything by zero!!! Its infinity");//something like this
}

or to be more clear:

try {
  sum=(a1/x)+(a1/x)+(a3/x)+...;
}
catch (Exception e) {
    syso("Hey man! You cannot divideanything by zero!!! Its infinity");//something like this
    sum=(a1/y)+(a1/y)+(a3/y)+...;
}
syso(sum);

Note the where syso(sum) is placed

Hope it gives you a bit clear idea :)

SteeveDroz
  • 665
  • 3
  • 8
  • 15
Samir Khanal
  • 1