Transferring data from one file stream to another

I am currently reading Clean Code and for fun and profit, I intended to refactor some of the Java Tutorials and since I wanted to improve my Exception handling, the Byte Stream one with its lots of IOExceptions seemed nice.

This code does work, but is it better now? As far as I can tell it's more readable now, though there are much more methods. Can someone more experienced with clean code tell me what to improve on this?

This code just copies bytes from one file to another:

```
public class Tutorial {

public static void copyBytes() {

FileInputStream input = getNewFileOutputStream();
FileOutputStream output = getNewFileInputStream();

try {
transferBytesFrom(input, output);
} finally {
closeStreams(input, output);
}
}

private static void closeStreams(FileInputStream input, FileOutputStream output) {
try {
input.close();
output.close();
} catch (IOException e) {
e.printStackTrace();
}
}

private static void transferBytesFrom(FileInputStream input, FileOutputStream output) {
int singleByte;

while ((singleByte = readByteFrom(input)) != -1) {
writeByteTo(output, singleByte);
}
}

private static void writeByteTo(FileOutputStream output, int singleByte) {
try {
output.write(singleByte);
} catch (IOException e) {
e.printStackTrace();
}
}

private static int readByteFrom(FileInputStream input) {
try {
return input.read();
} catch (IOException e) {
e.printStackTrace();
System.out.println(e.getMessage());
}

return -1;
}

private static FileOutputStream getNewFileInputStream() {
try {
return new FileOutputStream("output.txt");
} catch (FileNotFoundException e) {
e.printStackTrace();
System.out.println(e

Work with top level classes

Currently, your code only works for file related operations, as seen by the signature of transferBytesFrom.

transferBytesFrom(FileInputStream input, FileOutputStream output)

However, such an operation ought to work for every InputStream and every OutputStream, not just for file streams.

As such, it would be preferable to make all of your methods as generic as possible and let them take Input/OutputStream as arguments, or let them return Input/OutputStream:

private static void transferBytesFrom(InputStream input, OutputStream output)

private static void writeByteTo(OutputStream output, int singleByte)

private static int readByteFrom(InputStream input)

private static InputStream getNewFileOutputStream()

// ... the same for the other methods

The important goal here is that a code as generic as possible is a code that can be reused, and also a code that can evolve more simply. If you were to change your requirement and make your class handle other types of streams (and not files), you would have to rewrite everything.

try-with-resources

Starting with Java 7, you can use the try-with-resources statement to handle the automatic closing of the resources. This will prevent you from any tricky bugs, like this one:

private static void closeStreams(FileInputStream input, FileOutputStream output) {
    try {
        input.close();  // <--- imagine this throws an exception
        output.close();
    } catch (IOException e) {
        e.printStackTrace();  // <--- we go here. Ooops! output was never closed!
    }
}

Using this statement, you can have

public static void copyBytes() {
    try (
        InputStream input = getNewFileOutputStream();
        OutputStream output = getNewFileInputStream();
    ) {
        transferBytesFrom(input, output);
    } catch (IOException e) {
        // do something meaningful here
    }
}

This will ensure that whatever happens inside the try block, each stream is properly closed.

This also means that your closeStreams isn't really needed, because it will be done automatically.

Separation of concern

Let's look at your 2 methods getNewFileOutputStream and getNewFileInputStream. Their goal, as stated by their (good!) name is to retrieve a FileOutputStream and a FileInputStream. However, they currently do more than that. They currently handle the exceptions that can happen, and return null in that case.

Returning null when there is an exception is generally a code-smell: it indicates that something wrong happened, the method where this went wrong doesn't know what to do so it returns null. This creates bugs and, in fact, you have one in that case: if the files can't be opened for reading / writing, you will throw a NullPointerException later on. This is a problem: you want to fail as fast as possible and not let your code crash a couple of methods down with an unrelated and sometimes hard to debug exception, when it could have handled it before that.

So in this case, do not return null when there is an exception. In fact, I would let the method throw the checked exception. Its concern was "get me a InputStream; if it can't do that, it should throw.

In light of that, you can refactor your method to:

private static InputStream getInputStream() throws IOException {
    return new FileInputStream("xanadu.txt");
}

There are important comments here:

• The method was renamed getInputStream: the global goal here is to copy bytes between streams and this goes in-line with the first point; we don't need to restrict ourselves to files. Let's go all the way and be as generic as possible.

• The method returns the generic InputStream instead of the specific FileInputStream for the same reason.

• The method declares to throws IOException: it isn't its purpose to decide what to do when an input stream can't be retrieved so it lets this condition be handled by whoever is calling it. Note that in this case, it will be handled automatically by the try-with-resources introduced earlier.

Essentially, the same comments apply for your other methods, so you could have:

private static void writeByteTo(OutputStream output, int singleByte) throws IOException {
    output.write(singleByte);
}

private static int readByteFrom(InputStream input) throws IOException {
    return input.read();
}

Putting it all together

With all those modifications, you would have:

```
public static void copyBytes() {
try (
InputStream input = getInputStream();
OutputStream output = getOutputStream();
) {
transferBytesFrom(input, output);
} catch (IOException e) {
// do something meaningful here
}
}

private static void transferBytesFrom(InputStream input, OutputStream output) throws IOException {
int singleByte;
while ((singleByte = readByteFrom(input)) != -1) {
writeByteTo(output, singleByte);
}
}

private static void writeByte

StackExchange Code Review Q#125488, answer score: 2