5

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.getForeignWord() + " / " + card.getNativeWord();
            try {
                writter.write(line);
            } catch (IOException e) {
                e.printStackTrace();
            }
        }
        try {
            writter.flush();
            writter.close();
        } catch (IOException e) {
            e.printStackTrace();
        }
    }
Chadwick
  • 12,555
  • 7
  • 49
  • 66
Eugene
  • 59,186
  • 91
  • 226
  • 333

6 Answers6

11

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.

T.J. Crowder
  • 1,031,962
  • 187
  • 1,923
  • 1,875
  • @T.J. Crowder why do you close `writer` inside `try` block? Isn't it redundant? – Andrei Андрей Листочкин Nov 14 '10 at 17:33
  • 1
    @Andrew: *"Isn't it redundant?"* No, because when I close it in the `finally` block, I eat (suppress) any exception that occurs when closing it. When I close it in the main block, I don't -- because if it throws I want that to propagate. This is only one way to do it; another is to have some kind of success/fail flag you check; another is to have a final `catch` that catches *everything*, does the silent cleanup, and then rethrows. I usually find just doing the close in the main block simpler, esp. as I have utility functions for doing the silent closes that make them one-liners. – T.J. Crowder Nov 14 '10 at 18:39
  • @T.J. Crowder Thanks, now I see your point! Previously I used the pending-exception idiom, described here: http://stackoverflow.com/questions/884007/best-way-to-close-nested-streams-in-java/884182#884182 (ignore all that `Closeables` management) although I don't wrap it into a separate class. I end up with a code like this: https://gist.github.com/676312 How does it compare to your idiom? Oh, and why do you set `writer` to null inside the finally block? – Andrei Андрей Листочкин Nov 15 '10 at 01:45
  • 1
    @Andrew: It looks more complicated than what I usually end up with, and it highlights the issue of rethrowing the exception and what type of exception you rethrow. I don't want to convert everything into `RuntimeException` s. Here's what that code looks like the way I usually write it: http://pastie.org/1298959 or more accurately http://pastie.org/1298961 or even http://pastie.org/1298962 if I'm willing to rely on on-the-fly inlining in HotSpot. :-) – T.J. Crowder Nov 15 '10 at 06:55
1

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.

duffymo
  • 305,152
  • 44
  • 369
  • 561
1

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.

khachik
  • 28,112
  • 9
  • 59
  • 94
0

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

Peter
  • 47,963
  • 46
  • 132
  • 181
0

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

Fortyrunner
  • 12,702
  • 4
  • 31
  • 54
0

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
    }
}
ILMTitan
  • 10,751
  • 3
  • 30
  • 46