4
\$\begingroup\$

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;
}
\$\endgroup\$
2
  • \$\begingroup\$ Welcome to Code Review! I hope you get some great answers. \$\endgroup\$ – Phrancis May 1 '18 at 20:58
  • \$\begingroup\$ Thank you, I am a noobie to Code Review, so please forgive me! \$\endgroup\$ – Kyle May 1 '18 at 21:00
3
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Could you talk about the throwing of an unchecked exception or log a little more? \$\endgroup\$ – Kyle May 1 '18 at 22:04
  • \$\begingroup\$ You are welcome :) I was updating my answer until I though there must be an answer for this on StackOverflow, so I added a link for clarification. It is explained in depth over there! \$\endgroup\$ – Koekje May 1 '18 at 22:44
1
\$\begingroup\$
  • 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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.