2
\$\begingroup\$

Does this piece of code make sense? The idea is to throw any exceptions that may occur but always run the finally block to close the streams.

private void streamToFile(HttpResponse transferResponse) throws Exception {
    OutputStream output = null;
    InputStream input = null;
    try {
        input = new BufferedInputStream(transferResponse.getEntity().getContent());
        output = new FileOutputStream(transferFile);
        final int BUFFER_SIZE = 1024*10;
        byte data[] = new byte[BUFFER_SIZE];
        int count;
        while ((count = input.read(data)) != -1) {
            output.write(data, 0, count);
            publishProgress(count);
            remainingBytes = remainingBytes - count;
        }
    } finally {
        input.close();
        output.close();
    }
}
\$\endgroup\$
  • 4
    \$\begingroup\$ This is what a finally block is for. \$\endgroup\$ – Konrad Rudolph Jul 9 '12 at 14:29
  • \$\begingroup\$ It runs the code in all circumstances. ie weather the exception is thrown or not \$\endgroup\$ – jiduvah Jul 9 '12 at 15:03
  • \$\begingroup\$ My comment wasn’t a question, it was a statement. \$\endgroup\$ – Konrad Rudolph Jul 9 '12 at 15:19
  • \$\begingroup\$ Sorry Konrad, its not so clear. What is? \$\endgroup\$ – jiduvah Jul 9 '12 at 15:30
10
\$\begingroup\$

You could use the closeQuietly methods of IOUtils (Apache Commons IO) (or similar ones) for input streams. It ignores exceptions and accepts null values.

Its implementation is just a few lines of code, so you can copy it directly to your Android application without the jar file:

public static void closeQuietly(Closeable closeable) {
    try {
        if (closeable != null) {
            closeable.close();
        }
    } catch (IOException ioe) {
        // ignore
    }
}

You could also create a close method for ouput streams:

public static void closeQuietly(OutputStream stream) throws IOException {
    if (stream != null) {
        stream.close();
    }
}

Both methods accept null values which means that you can call them with null, closeQuietly and close will handle that and you don't have to check null values in the finally block:

} finally {
    if (input != null) {
        closeQuietly(input);
    }
    close(output);
}

Instead of this the following is completely enough:

} finally {
    closeQuietly(input);
    close(output);
}

It's much simpler.

closeQuietly ignores exceptions which means that it catches them but don't do anything with them. Notice the empty catch block:

    } catch (IOException ioe) {
        // ignore
    }

It ensures that if closeable.close() throws an IOException the close(output) will run and it will close the output stream too.

I'm a little bit paranoid about this so I always put a logging statement to the catch block:

    } catch (IOException ioe) {
        logger.warn("Could not close the stream", ioe);
    }

To be honest, I've never seen this exception, so I think it's safe to left this block empty.

Ignoring IOExceptions of output streams could be dangerous: you might miss a bug and get a corrupted output file.

\$\endgroup\$
  • \$\begingroup\$ its actually Android so thats not really an option \$\endgroup\$ – jiduvah Jul 9 '12 at 15:01
  • \$\begingroup\$ @jiduvah: check the update, please. \$\endgroup\$ – palacsint Jul 9 '12 at 15:36
  • \$\begingroup\$ what is the point of this code? You mentioned that I should use closeQuietly methods as it ignores exceptions and accepts null. But here you are catching an exception and checking for null \$\endgroup\$ – jiduvah Jul 9 '12 at 15:44
  • 2
    \$\begingroup\$ ... so you include this function, and call it from your finally blocks - instead of checking for nulls and exceptions and calling .close(). Seems clear enough for me - not for you? \$\endgroup\$ – ANeves wants peace for Monica Jul 9 '12 at 15:48
  • 1
    \$\begingroup\$ You should NOT use closeQuietly, surely not for outputs streams, as it's unsafe. Closer is the replacement (or @Cleanup if you can use Lombok). \$\endgroup\$ – maaartinus May 30 '14 at 4:50
4
\$\begingroup\$

Yes this is the right way to do it but you should check output and input variables for null value before calling on them the close() method.

\$\endgroup\$
  • \$\begingroup\$ ... and also change it to something like try {close1(); } finally {close2()}, so that both get executed. \$\endgroup\$ – maaartinus Jun 3 '14 at 5:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.