views:

245

answers:

6

All,

I am trying to ensure that a file I have open with BufferedReader is closed when I catch an IOException, but it appears as if my BufferedReader object is out of scope in the catch block.

public static ArrayList readFiletoArrayList(String fileName, ArrayList fileArrayList)
{
    fileArrayList.removeAll(fileArrayList);

    try {
        //open the file for reading
        BufferedReader fileIn = new BufferedReader(new FileReader(fileName));

        // add line by line to array list, until end of file is reached
        // when buffered reader returns null (todo). 
        while(true){
                fileArrayList.add(fileIn.readLine());
            }
    }catch(IOException e){
        fileArrayList.removeAll(fileArrayList);
        fileIn.close(); 
        return fileArrayList; //returned empty. Dealt with in calling code. 
    }
}

Netbeans complains that it "cannot find symbol fileIn" in the catch block, but I want to ensure that in the case of an IOException that the Reader gets closed. How can I do that without the ugliness of a second try/catch construct around the first?

Any tips or pointers as to best practise in this situation is appreciated,

A: 

Move the declaration out of the try block:

public static ArrayList readFiletoArrayList(String fileName, ArrayList fileArrayList)
{
    fileArrayList.removeAll(fileArrayList);

    BufferedReader fileIn = null;
    try {
        //open the file for reading
        fileIn = new BufferedReader(new FileReader(fileName));

        // add line by line to array list, until end of file is reached
        // when buffered reader returns null (todo). 
        while(true){
                fileArrayList.add(fileIn.readLine());
            }
    }catch(IOException e){
        fileArrayList.removeAll(fileArrayList);
        fileIn.close(); 
        return fileArrayList; //returned empty. Dealt with in calling code. 
    }
}

But, you'll still need to be careful that fileIn was actually initialized before trying to close it:

if (fileIn != null)
    fileIn.close();
Kaleb Pederson
If there's an exception in the reader constructors (file not found, for instance), that'll blow up with a `NullPointerException`. Have to check the `null` in the `catch`.
T.J. Crowder
+1  A: 

Once you hit the catch block, any variables declared in the try are not scoped anymore. Declare BufferedReader fileIn = null; above the try block, then assign it inside. In your catch block, do if(fileIn != null) fileIn.close();

purecharger
+1  A: 

It's complaining about the symbol not being there because it's not. It's in the try block. If you want to refer to fileIn, you'll need to declare it outside the try.

However, it really sounds like you'd want to place the close in a finally block instead: you should close the file regardless of success or failure before returning.

public static ArrayList readFiletoArrayList(String fileName, ArrayList fileArrayList)
{
    fileArrayList.removeAll(fileArrayList);

    BufferedReader fileIn = null;
    try {
        //open the file for reading
        fileIn = new BufferedReader(new FileReader(fileName));

        // add line by line to array list, until end of file is reached
        // when buffered reader returns null (todo). 
        while(true){
                fileArrayList.add(fileIn.readLine());
            }
    }catch(IOException e){
        fileArrayList.removeAll(fileArrayList); 
    }finally{
        if(fileIn != null) fileIn.close();
    }
    return fileArrayList;
}
Yuliy
+6  A: 
 BufferedReader fileIn = null;
 try {
       fileIn = new BufferedReader(new FileReader(filename));
       //etc.
 } catch(IOException e) {
      fileArrayList.removeall(fileArrayList);
 } finally {
     try {
       if (fileIn != null) fileIn.close();
     } catch (IOException io) {
        //log exception here
     }
 }
 return fileArrayList;

A few things about the above code:

  • close should be in a finally, otherwise it won't get closed when the code completes normally, or if some other exception is thrown besides IOException.
  • Typically you have a static utility method to close a resource like that so that it checks for null and catches any exceptions (which you never want to do anything about other than log in this context).
  • The return belongs after the try so that both the main-line code and the exception catching have a return method without redundancy.
  • If you put the return inside the finally, it would generate a compiler warning.
Yishai
+1 for the `finally`. Surprisingly a lot of answers didn't mention about it. Here are some Sun links of interest: [Lesson: Basic IO](http://java.sun.com/docs/books/tutorial/essential/io/) and [Lesson: Exceptions](http://java.sun.com/docs/books/tutorial/essential/exceptions/index.html)
BalusC
You can remove the check for `null` and catch `Exception`, unless you plan on telling the user that you couldn't close the file versus couldn't open it. But, generally, users don't care.
Dave Jarvis
You closed the BufferdedRead, but not the FileReader?
Bert F
Closing the BufferedReader will automatically close the FileReader.
DaveJohnston
@Bert: Indeed, the Java IO classes follows the decorator pattern. So will under each `close()` delegate upwards to the decorated instance.
BalusC
@Dave Jarvis, it is certainly a reasonable alternative, but I think the above represents the "standard" best practice. But as long as you log the exception, then I think it doesn't really matter.
Yishai
A: 

Declare the BufferedReader outside the try block and set it to null then use a finally block to close it if its not null. Also fileArrayList is passed by reference so any changes made to it will happen to the object you passed in so there is no need to also return it.

    public static ArrayList readFiletoArrayList(String fileName, ArrayList fileArrayList)
{
    fileArrayList.removeAll(fileArrayList);
    BufferedReader fileIn = null;
    try {
        //open the file for reading
        fileIn = new BufferedReader(new FileReader(fileName));

        // add line by line to array list, until end of file is reached
        // when buffered reader returns null (todo). 
        while(true){
                fileArrayList.add(fileIn.readLine());
            }
    }catch(IOException e){
        fileArrayList.removeAll(fileArrayList);  
    }finally
    {
       try
       {
           if(fillIn != null)
               fileIn.close();
       }
       catch(IOException e){}
    }
    return fileArrayList; //returned empty. Dealt with in calling code.
}
John ClearZ
+1  A: 

My preferred way of performing clean-up after an exception (when the clean-up can potentially also throw an exception) is to put the code in the try block inside another try/finally block, as follows:

public static ArrayList readFiletoArrayList(String fileName, ArrayList fileArrayList) {
    fileArrayList.removeAll(fileArrayList);

    try {
        //open the file for reading
        BufferedReader fileIn = null;

        try {
            fileIn = new BufferedReader(new FileReader(fileName));
            // add line by line to array list, until end of file is reached
            // when buffered reader returns null (todo). 
            while(true){
                fileArrayList.add(fileIn.readLine());
            }
        } finally {
            if (fileIn != null) {
                fileIn.close();
            }
        }
    }catch(IOException e){
        fileArrayList.removeAll(fileArrayList);
        return fileArrayList; //returned empty. Dealt with in calling code. 
    }
}
DaveJohnston