Should multiple Try/Catch blocks in a method be combined

Looking on my method I'm not sure I have to use three separate try/catch blocks. Should I use the only one for the whole method? What is a good practice?

public void save(Object object, File file) {    

        BufferedWriter writter = null;

        try {
            writter = new BufferedWriter(new FileWriter(file));
        } catch (IOException e) {
            e.printStackTrace();
        }

        Dictionary dictionary = (Dictionary)object; 
        ArrayList<Card> cardList = dictionary.getCardList();
        for (Card card: cardList) {
            String line = card.g开发者_高级运维etForeignWord() + " / " + card.getNativeWord();
            try {
                writter.write(line);
            } catch (IOException e) {
                e.printStackTrace();
            }
        }
        try {
            writter.flush();
            writter.close();
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

You certainly don't want three separate blocks with the code as it is. In the first block, you're catching an error setting up writer, but then in subsequent blocks you're using writer, which doesn't make sense if the first block failed. You'll end up throwing a NullPointerException when an I/O error occurs — not ideal. :-)

There's a lot of room for style in this stuff, but here's a fairly standard reinterpretation of your function. It only uses two blocks (though you may choose to add back a third; see the comments in the code):

public void save(Object object, File file) {    

    BufferedWriter writter = null;

    try {
        writter = new BufferedWriter(new FileWriter(file));

        Dictionary dictionary = (Dictionary)object; 
        ArrayList<Card> cardList = dictionary.getCardList();
        for (Card card: cardList) {
            String line = card.getForeignWord() + " / " + card.getNativeWord();
            writter.write(line); // <== I removed the block around this, on
                                 // the assumption that if writing one card fails,
                                 // you want the whole operation to fail. If you
                                 // just want to ignore it, you would put back
                                 // the block.
        }

        writter.flush(); // <== This is unnecessary, `close` will flush
        writter.close();
        writter = null;  // <== null `writter` when done with it, as a flag
    } catch (IOException e) {
        e.printStackTrace(); // <== Usually want to do something more useful with this
    } finally {
        // Handle the case where something failed that you *didn't* catch
        if (writter != null) {
            try {
                writter.close();
                writter = null;
            } catch (Exception e2) {
            }
        }
    }
}

Note on the finally block: Here, you may be handling the normal case (in which case writter will be null), or you might be handling an exception you didn't catch (which isn't unusual, one of the main points of exceptions is to handle what's appropriate at this level and to pass anything else up to the caller). If writter is !null, close it. And when you close it, eat any exception that occurs or you'll obscure the original one. (I have utility functions for closing things whilst eating an exception, for exactly this situation. For me, that could would be writter = Utils.silentClose(writter); [silentClose always returns null]). Now, in this code you may not be expecting other exceptions, but A) You may change that later, and B) RuntimeExceptions can occur at any point. Best to get used to using the pattern.

What is good depends on what your code is intended to do. For example, the second try/catch is inside a loop, which means, it will process all cards in the list, even if processing of one is failed. If you use one try/catch for the whole method, this behavior will be changed. Use one try/catch if your algo allows.

It's a matter of taste - there's no right answer.

Personally, I prefer one try/catch block. If I see lots of them, I start worrying that my method is doing too much and getting too big. It's a sign that it might be time to break it into smaller methods.

Here's how I'd write that method:

public void save(Object object, File file) throws IOException
{

    BufferedWriter writer  = null;

    try {
    writer = new BufferedWriter(new FileWriter(file));

    Dictionary dictionary = (Dictionary) object;
    ArrayList<Card> cardList = dictionary.getCardList();
    for (Card card : cardList)
    {
        String line = card.getForeignWord() + " / " + card.getNativeWord();
        writer.write(line);
    }
    }
    finally
    {
        IOUtils.close(writer); 
    }    
}

I'd have an IOUtils class that would encapsulate the proper closing of streams, readers, and writers. You'll write that code again and again. I'd make it a one-liner as a static method in a class - or use the Apache Commons IO Utils JAR if I didn't mind another dependency.

With try-catch, there are more 'don't s' than 'do's' I'm afraid...

This document lists some of them :

http://today.java.net/article/2006/04/04/exception-handling-antipatterns

Nevertheless, there are some do's out there, I found this interesting :

http://www.wikijava.org/wiki/10_best_practices_with_Exceptions

In the specific case of I/O I suggest that you look at some of the common libaries available such as Guava and Jakarta commons. They have some nice scripts for performing silent closes.

E.g. Commons IO IOUtils

These can clean up your code quite a bit

I prefer to write two try blocks, one for the finally and one for the catch. Because of the way it is written, you don't ever set writter to null, nor do you have to check against null when closing. The only problem is an exception while closing would hide an exception while processing, but it is only a problem if you want to handle the two exceptions differently.

public void save(Object object, File file) {
    try {
        BufferedWriter writter = new BufferedWriter(new FileWriter(file));
        try {
            Dictionary dictionary = (Dictionary)object; 
            ArrayList<Card> cardList = dictionary.getCardList();
            for (Card card: cardList) {
                String line = card.getForeignWord() + " / " + card.getNativeWord();
                writter.write(line);
            }
        } finally {
            writter.flush();
            writter.close();
        }
    } catch (IOException e) {
        e.printStackTrace(); //Or something more useful
    }
}