trjhtr

To help improve my proficiency with Java, I would like to know how should I close these FileReaders as I have come across multiple answers, and what could I do better with this block of code?

/**
 * Returns a Vector<String> object that that can be parsed into classes.
 * The file arugment must specify a realative or aboustle adress to the file.
 * @param file to read
 * @return
 */
public static ArrayList<String> read(File file) 

 ArrayList<String> data = new ArrayList<String>();

 String lineRead;

 try 

 FileReader fr = new FileReader(file);
 BufferedReader br = new BufferedReader(fr);

 while ( (lineRead = br.readLine()) != null ) 
 data.add(lineRead); 
 

 br.close();
 fr.close();

 catch(IOException e) 
 e.printStackTrace();
 

 return data;

Since Java 7, you can make use of try-with-resources, which would change the implementation to:

try (BufferedReader br = new BufferedReader(new FileReader(file))) 
 while ((lineRead = br.readLine()) != null) 
 data.add(lineRead);
 
 catch (IOException e) 
 ...

Some other tips:

• Your documentation is not telling the truth and incomplete, you have to add some return documentation. Also, for parameters, the first string represents the name, and what follows is the explanation. So it should be more like e.g. @param file the file to read all lines from.


• Return the interface instead of the implementation, i.e. return List instead of ArrayList, which means you can always change the implementation.


• 
  

  Never just print the stack trace, wrap and throw an (unchecked) exception or log. For more information as to why, see why printStackTrace is considered bad practice. In case you want to propagate the exception nonetheless, you can wrap it in another exception, e.g.:

  
  
  

  try 
   ...
   catch (IOException e) 
   throw new UncheckedIOException(e);
  
  

  
  

This example may be used when the method this code is executed in does not declare to throw a checked Exception. But it all depends on the case at hand.

• 
  

  BufferedReader has a lines method returning all String lines as a stream. With this, you could also implement it as follows:

  
  
  

  return br.lines().collect(toList());
  

  
  

I statically imported Collectors#toList. Notice how this results in a List and not an ArrayList.

• First of all, your call to fr.close() has no effect, because fr has already been closed by br.close(), since close() releases all underlying resources, which in this case includes other streams or readers that br is wrapped around.



• 
  

  Also, and more importantly, it is imperative that br.close() is called regardless of whether the try block succeeds or throws an exception. In your code, if br.readLine() throws an exception, the reader will never be closed.

  
  
  

  To rectify this, the call to br.readLine() has to go into a finally block instead of the try block, which would mean that br has to be declared outside the try block:

  
  
  

  BufferedReader br = null; //need to initialize it here, or else the compiler will complain later
  try 
   FileReader fr = new FileReader(file);
   br = new BufferedReader(fr);
   //...
   catch (IOException e ) 
   //...
   finally 
   br.close();
  
  

  
  
  

  And this is what the try-with-resources statement mentioned by Koekje is a shortcut for.