Most of this was written before your second post; I've left it intact even though some no longer applies. Let's forget about efficiency for a minute. What happens if an IOException occurs? The error is caught and ignored, and "Image downloaded!!!" is printed. Bad, very bad. Never ever ignore an error like this. Even for your other error handling code, it would be preferable to print a stack trace, unless you know in advance exactly why and where the error occurrec. Well, for FileNotFoundException you don't need a stack trace - but printing the name of the file would certainly be helpful. And why is "Image downloaded!!!" in a finally clause? Shouldn't it only appear if the image is actually downloaded, with no errors? Next (still ignoring efficiency), this code has a good chance of corrupting the image data. That's because it uses Readers and Writers which assume that the data is text data. It's not. Various subtle errors can occur because of this. Most commonly, there's more than on kind of line separator recognized by BufferedReader, and when you tell it to readLine() it treats these different separators as equivalent. Which is a problem if they weren't really equivalent because they weren't really separators, because they were bytes of image data that just happended to have the same values as '\r' or '\n'. Basically, Readers and Writers are for text. Images are not text. Therefore, don't use Readers and Writers for images. Use InputStreams and OutputStreams. Here's a simple way to copy bytes:
Or more efficiently:
That's about as efficient as you can get unless you use JDK 1.4's NIO classes. These are harder to master, and finding the most efficient technique will depend more on your platform and on the size of the files you're transferring; too complex for now. Of course these code examples need to have close() methods and error handling added; I kept them simple.
Thanks Jim. That's an amazing response. Reader and Writer were corrupting the downloaded image file. So I replaced them with Streams and it's working fine. Following is the current code I have :
I have two questions though, 1. Without the line bos.flush();, my image was still getting corrupted. Why is it so? 2. What would be the difference in efficiency between using Buffered streams and using manual byte buffer
1. Without the line bos.flush();, my image was still getting corrupted. Why is it so? Hmmm, hard to say for sure. Usuallyeach close() normally does a flush() too, so you normally don't need to insert this yourself unless you want to ensure the bytes are flushed in cases where the stream has not yet been closed. E.g. if you want to see intermediate results while a file is still printing, or if you want to ensure output is flushed even when to program is terminated prematurely. (Thing like System.exit() and ctrl-C may not allow the JVM to flush streams before exiting.) However, the API for OutputStream doesn't actually guarantee that a flush is done() as part of a close(). It's performed bya FilterOutputStream, as well as by any Writer. But it's not guaranteed for OutputStreams in general. So I guess we really should get in the habit of flushing an OutputStream before closeing it. Is it possible that an error was thrown somewhere in the finally statement, before the bos.close()? I often favor putting different stream closures in separate try-catch blocks, even separate methods, to be safe. E.g.
This can be called from your fetchContent method, which now doesn't have to worry about the output streams at all. It's easy to see that the stream is flushed and closed int the same method that opens it, with no chance of any unrelated exceptions preventing the close from being called. (If an IOException occurs I don't care so much about flushing, but I care that the file is not held open, because sometimes the subequent code is going to delete or move the file, and open file streams interfere with this.) Note that the way this is structured, there's no need to check if out == null. If the constructor complete successfully, out is not null, and the close() will be called. If an exception is thrown during contruction, out will not have been constructed, and it's neither necessary nor possible to close it. You can add as much complex error handling as you need to here. In many cases I might want to throw or re-throw an exception to indicate the file write did not occur. Here I just caught and logged the problem. You've got many possible options. The point here is to deal with the output steam in one method, separate from error handling for the input stream. Error handling gets messy and unreadable if you're dealing with too many things at once, and the chance of mistakes increases dramatically. 2. What would be the difference in efficiency between using Buffered streams and using manual byte buffer They're pretty close really. I tend to use manual buffering; it's a bit faster in most cases. If I'm moving a lot of data and a bulk read or write is available, I use it. I think of that as a standard paradigm that everyone who uses Java IO should learn. Most of the time it doesn't really matter much - the main benefit is in having some sort of buffer, somewhere. I suppose I could have omitted my "more efficient" example since you were already using buffered streams. But manual buffering is beneficial often enough that I think programmers should know how to use it.