CODEWITHSUNDEEP
javaexception
MasterJoe
MasterJoe

Reputation: 2335

How to handle exceptions when reading files with buffered reader?

I have made code to read text (json, xml etc.) from a text file, and convert it into a strings which can then be used by other code to convert into plain old java objects or POJOS that are annotated by jackson.

I am not sure if my code handles the exceptions properly. So far, I have used the principles mentioned after my code (READ THIS DOWN VOTERS !!!), to develop the code. Note that I cannot use try with resources because I am stuck with Java 6 (even though my project JRE is 1.8).

package com.testing;
import java.io.BufferedReader;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;

public class JunkEx {

    public static void main(String[] args) {
        String filePath = ".\\src\\test\\resources\\text-files\\orders\\orders-2017.txt";
        String contents = fileToString(filePath);
        System.out.println(contents);
    }

    private static String fileToString(String filePath) {
        StringBuilder stringBuilder = null;
        BufferedReader br = null;

        try {
        br = new BufferedReader(new FileReader(filePath));
        stringBuilder = new StringBuilder();
        String currentLine;
        while ((currentLine = br.readLine()) != null) {
            stringBuilder.append(currentLine);
            stringBuilder.append("\n");
        }

        }catch (FileNotFoundException ex1) {
            ex1.printStackTrace();
        }catch (IOException ex2) {
            ex2.printStackTrace();
        }finally {
            try {
                br.close();
            } catch (IOException e) {
                e.printStackTrace();
            }
        }
        return stringBuilder.toString();
    }

}

Principles:

1) Catch the most specific exception first, then the one above it in the exception hierarchy. I.e catch FileNotFoundException 1st and IOException later. Refer point 5 here

2) Do NOT return from inside a finally block, because finally is always executed as long as try "completes" fully or abruptly. Refer this SO answer.

3) Cleanup resources like buffered readers in the finally block. Refer point 1 here.

4) Do not make the callers of the "dangerous" method (i.e. which could throw exceptions), have to know/throw each of the exceptions inside it. I.e dangerous method should not "throws FileNotFoundException, IOException...etc". refer this link, specifically the last paragraph

Flaw in the code: If any of the first two catch blocks are executed, then its likely that the entire file was not read. But, my method will return a string anyway. The string could be null or incomplete.

Questions -

1) I want to throw an exception when the text file is not successfully converted to a string, i.e. one of the three catch blocks is executed. Should I wrap the exception in each catch block inside a generic Exception object and throw that or do something else ?

2) How can I improve/fix the exception handling of this code?

Project structure: enter image description here

Upvotes: 1

Views: 5141

Answers (2)

user207421
user207421

Reputation: 310936

Simple answer: don't catch the exceptions at all. Let the caller catch them. He's the one who needs to know, and knows what to do about them. This method certainly doesn't.

4) Do not make the callers of the "dangerous" method (i.e. which could throw exceptions), have to know/throw each of the exceptions inside it. I.e dangerous method should not "throws FileNotFoundException, IOException...etc". refer this link, specifically the last paragraph.

I don't think much of this 'principle', and in any case it doesn't actually apply here. The only exceptions thrown are IOException and FileNotFoundException, which is derived from it. The caller doesn't have to deal with both if he doesn't wish to. However he may wish to do exactly that, if for example the file being missing constitutes a deployment error.

Upvotes: 2

Jim Garrison
Jim Garrison

Reputation: 86774

You have two options:

  1. Don't catch the exceptions and let the caller deal with them
  2. Catch the exceptions and throw your own custom exception, say FileToStringFailedException, that wraps the actual exception. The wrapping part is extremely important, so that when someone up the call chain does a printStackTrace() you get a Caused By section that explains the real failure.

Either option works, and which one you choose depends on the overall design. For something simple I'd just let the exceptions percolate upwards; if I was designing a large complex library with other custom wrapper exceptions I might choose option 2.

Without a lot more context about what you're doing and where this fits into your system, it's not possible to say one option is "better".

Whatever you do, NEVER interfere with the actual exception. Exceptions are for programmers, not users. If you want "friendly" errors for users, do that at the point where the exceptions are displayed to the users, but make sure the original exceptions are logged for the programmers. otherwise you will regret it severely when your "friendly" exception hides crucial information that you need to troubleshoot a problem.

Upvotes: 1

Related Questions