Transferring data from one file stream to another

Question

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.getMessage());
 }
 return null;
 }
 private static FileInputStream getNewFileOutputStream() {
 try {
 return new FileInputStream("xanadu.txt");
 } catch (FileNotFoundException e) {
 System.out.println("ERROR: Input file not found!");
 }
 return null;
 }
}

Answer 1

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:

1. 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.
2. The method returns the generic InputStream instead of the specific FileInputStream for the same reason.
3. 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 writeByteTo(OutputStream output, int singleByte) throws IOException {
 output.write(singleByte);
}
private static int readByteFrom(InputStream input) throws IOException {
 return input.read();
}
private static OutputStream getOutputStream() throws IOException {
 return new FileOutputStream("output.txt");
}
private static InputStream getInputStream() throws IOException {
 return new FileInputStream("xanadu.txt");
}

Each method now has a clear goal and doesn't handle unnecessary exceptions. All potentials exceptions are left for the main method copyBytes to handle. Arguably, this is where it should be: this is the method responsible for organizing the work. A user calling that method shouldn't have to handle an IOException: the streams were opened and closed by this method alone, so it should be the one handling the exceptional circumstances; a caller shouldn't have to handle an exception for a scenario it didn't create in the first place.

There's still the question of what this method should do in the case of an exception. This answer really depends on your requirement then:

• it could rethrow a custom exception like CopyFailedException and have as cause the IOException. The advantage is that this exception isn't coupled with the fact we're doing IO here. A caller doesn't know that we're doing IO, but it knows that we're copying something, so it makes sense to tell the caller that the copy operation failed with a custom exception.
• it could log the error and do nothing else. The logging ensures that the exception is properly traced somewhere.

---

As a side-note, when you will reach the NIO.2 API in the Java tutorials, you will find that all of this code can be replaced with a simple call to Files.copy(source, target)

Files.copy(Paths.get("xanadu.txt"), Paths.get("output.txt"));

This method will directly copy the path "xanadu.txt" into "output.txt". It also takes a 3rd argument as a vararg that corresponds to CopyOptions. A useful one could be StandardCopyOption.REPLACE_EXISTING that tells the method to replace the target file if it already exists.

• 200_success: I disagree with the idea that the caller shouldn't have to handle IOException. The rule should be, if you don't have a good way to handle the exception, propagate it and let the caller deal with it. Here, the caller asked for a file to be copied; if something goes wrong that causes the copy to fail (permissions, disk space, etc.) then it's essential to let the caller know. And an IOException is the most logical way to communicate that fact.
• Tunaki: @200_success My point was that it shouldn't handle IOException but a more generic exception, that would contain the IOException as cause. The caller asked to copy bytes, this doesn't necessarily involve I/O operations. The bytes could be in memory.
• AdHominem: Thanks a lot for the detailed answer, I learned a lot! But two questions remain: 1. If the try-with-clause catches any IOException, both getInputStream and getOutputStream are redundant, or not? Because I mainly created them to encapsulate the ugly try-catch block, if they throw, why shouldn't I initialize them directly in the copyBytes function?
• AdHominem: 2. In your implementation, you throw all Exceptions in the lower functions to copyBytes. Isn't it better to catch Exceptions as early as possible? If an Exception occurs in writeByteTo, it is thrown to transferBytesFrom which again throws it to copyBytes. So I guess I will just get the message that copyBytes caught an IOException and no information that the cause was in writeByteTo?
• Tunaki: @AdHominem 1. getInputStream is supposed to return a inputstream. You might argue the necessity of having such method. It depends on whether you want to abstract the fact that we're dealing with files: with what we have now copyBytes really doesn't know that it's copying bytes from a file, all it has a inputstream. If later you want to read bytes from elsewhere, copyBytes won't change, only getInputStream. 2. No, exceptions should be catched by whoever is able to handle them properly. And the stacktrace will tell exactly where the exception happened.