FileIO lines to String Converter

Question

I think I made a good FileIO object because I can simply copy-paste whenever I need, however I wonder How I can improve my code.

This code takes file path of the target file and turns every line to Strings. And combine all of the Strings into String array so that we can use the data of the file.

import java.io.BufferedReader;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStreamReader;

public class FileIO {

private BufferedReader br;
private FileInputStream fIn;
private String fileName;
private int length;
private String[] data;

public FileIO(String fileName) throws IOException {
    this.fileName = fileName;
    createFile();
    setLength();
    setData();
    fIn.close();
    br.close();
}

private void createFile() {
    try {
        fIn = new FileInputStream(fileName);
        br = new BufferedReader(new InputStreamReader(fIn));} 
    catch (FileNotFoundException e) {
        System.out.println("ERROR! " + fileName + " named file does not found!");
        System.exit(-1);
    }}

private void setLength() throws IOException {
    length = 0;
    br.readLine(); // if first line of the file is not title, delete this.
    while((br.readLine()) != null) {
        length++;
    }}

private void setData() throws IOException {
    data = new String[length];
    fIn.getChannel().position(0);
    br = new BufferedReader(new InputStreamReader(fIn));
    br.readLine(); // if first line of the file is not title, delete this.
    for (int i = 0; i < length; i++) {
        data[i] = br.readLine();}
    }

public int getNumberOfLines() {
    return length;
}

public String[] getAllData() {
    return data;
}

public String getDataLine(int lineNumber) throws Exception {
    if (length < lineNumber) {
        throw new Exception("Line number is greater than all of the lines! Please enter a number between 0 and " + length);}
    return data[lineNumber-1];}
}

Answer 1

Java isn't my main language, but I noticed this:

catch (FileNotFoundException e) {
    System.out.println("ERROR! " + fileName + " named file does not found!");
    System.exit(-1);

• We should be writing the message to System.err, not System.out.
• It's normal to use small positive values for program exit status (-1 will likely end up converted to 255 in most shells).
• Consider whether there's extra information in the exception e that should be reported (file doesn't exist, exists but permission denied, etc).

---

Most of these members:

private BufferedReader br;
private FileInputStream fIn;
private String fileName;
private int length;
private String[] data;

are used only to communicate between functions called by the constructor:

public FileIO(String fileName) throws IOException {
    this.fileName = fileName;
    createFile();
    setLength();
    setData();
    fIn.close();
    br.close();
}

I think we can get rid of most of them with small changes to the functions so that they make those communications explicit:

private String[] data;

public FileIO(String fileName) throws IOException {
    FileInputStream fIn = openFile(fileName);
    BufferedReader br = new BufferedReader(new InputStreamReader(fIn));
    data = new String[measureLength(br)];
    fIn.getChannel().position(0);
    br = new BufferedReader(new InputStreamReader(fIn));
    readLines(br);
}

(Obviously, uses of length in the code are now replaced by data.length).

openFile(), measureLength() and readLines() can then all be static methods, making them more self-contained.

Answer 2

1. Where's your javadoc?
2. createFile() is misleadingly named. It does not create a file.
3. Similarly, setLength() and setData() are poor names - set<thing> methods in Java have an expected form, which these methods don't fit.
4. You'd do better to build a List (which will grow as necessary), probably an ArrayList, rather than reading the file twice. If you really want an array, you can get it from the List easily enough.

This is a well-trodden path - see for example this discussion

Answer 3

Some important bits were not mentioned:

Exception handling

• In your createFile you are catching the exception and exiting. If you want to use this as a command line tool I guess it's fine and this way you will get rid of the stacktrace, but otherwise I would just not catch the exception, because the program will also end with error, just also with the stacktrace. If this was a library, you don't want to shut down the program for your library user.
• Your check for line index, you are throwing general Exception, but there's IndexOutOfBoundsException specifically for these cases. If you are checking for upper bound you may as well check for the line to be above zero too.
• The whole getDataLine is redundant though anyway, since the same can be done by calling getAllData()[line] and it would already have proper index handling and exception when the index is out of bounds.

Stream closing

• You only need to close the most outer stream, so br.close(); is enough.
• If everything works out, your streams are closed okay. But imagine some exception happens before the close() lines and they never happend. This should be handled by proper try-catch-finally or try with resources pattern. Otherwise your program may end up with leaked unclosed streams.