How do I refactor closing a stream in Java?

Question

Due to my company's policy of using Eclipse and using Eclipse's code-autofix, the following code pattern appears excessively in the codebase:

InputStream is = null;
try {
    is = url.openConnection().getInputStream();
    // .....
} catch (IOException e) {
    // handle error
} finally {
    if (is != null) {
        try {
            is.close();
        } catch (IOException e) {
            // handle error
       }
    }
}

IMO it's extremely fugly and hard to read, especially the portion within the finally block (is there really a need to catch 2 instances of IOException?). Is there anyway to streamline the code such that it looks cleaner?

Note that I cant use external libraries such as Apache IOUtils not unless it goes testing and approval by multiple entities — david, Oct 22 '10 at 01:33
The whole point of using Apache IOUtils is that it has already been tested by countless other people. That being said, I feel your pain. Wouldn't it be worth it to have IOUtils tested and approved *once* within your organization so you can use it across all your projects? — Bill the Lizard, Oct 22 '10 at 01:36
If only life were that simple... I have to go through many levels of management and red tape to even get it seen by the working groups involved. And we're still stuck using JDK1.4.2 due to 'legacy' issues with software that has been retired ages ago — david, Oct 22 '10 at 01:43
@david: Print out a copy of this for your bosses and put it in their inbox: http://www.oracle.com/technetwork/java/eol-135779.html — andersoj, Oct 22 '10 at 01:51
I shudder to think what working in this environment might be like; one where the software developers are not trusted to make decisions about how to develop software — matt b, Oct 22 '10 at 02:25
http://stackoverflow.com/questions/588546/does-close-ever-throw-an-ioexception — TofuBeer, Oct 22 '10 at 03:05
Just throw the exception, it doesn't look like you can do anything meaningful with it anyway. — CurtainDog, Oct 22 '10 at 04:33

score 3 · Answer 1 · answered Oct 22 '10 at 05:02

3

Why do anything? It's working code. It's correct.

Leave it be.

answered Oct 22 '10 at 05:02

user207421

305,947
44
307
483

For the counter argument, http://www.klocwork.com/blog/2010/10/refactoring-if-it-aint-broken-dont-fix-it/ – andersoj Oct 22 '10 at 10:14
He didn't ask, "What should I do", he asked, "How do I do this." – Tony Ennis Oct 22 '10 at 17:44
And I am asking why. Why should there be a budget for disturbing working code? Working *generated* code at that. I regard generated code as object code personally, I don't mess with it. – user207421 Oct 23 '10 at 00:53

score 1 · Answer 2 · edited May 23 '17 at 12:14

See this question, use the closeQuietly() solution.

InputStream is = null;
try {
    is = url.openConnection().getInputStream();
    // .....
} catch (IOException e) {
    // handle error
} finally {
    IoUtils.closeQuietly(is);
}

// stolen from the cited question above
public class IoUtils {

  public static closeQuietly (Closeable closeable) {
    try {
      closeable.close();
    } catch (IOException logAndContinue) {
      ...
    }
  }
}

Or wait for JDK7's ARM blocks.

score 1 · Answer 3 · answered Oct 22 '10 at 01:40

1

First, about using IOUtils - may worth a shot telling your supervisors that the very application-server / Java runtime environment they might use, uses IOUtils and similar libraries itself. so in essence you're not introducing new components to your architecture.

Second, no, not really. There isn't really any way around it other than writing your own utility that will immitate IOUtils' closeQuietly method.

answered Oct 22 '10 at 01:40

Isaac

16,458
5
57
81

Heh, if closeQuietly is opensource, copy it into your project. – Tony Ennis Oct 22 '10 at 02:09
... And the inventor of reusability and modularity rests happily in his grave.... – Isaac Oct 22 '10 at 02:28
closeQuietly ignores the exception. That's never a good practice. Besides, david wants to "handle error". – Martín Schonaker Oct 22 '10 at 05:51

score 1 · Answer 4 · answered Oct 22 '10 at 01:42

public class Util {
    public static void closeStream(inputStream is) {
        if (is != null) {
            try {
               is.close();
            } catch (IOException e) {
               // log something
        }
    }
}

Now your code is

InputStream is = null;
try {
    is = url.openConnection().getInputStream();
    // .....
} catch (IOException e) {
    // handle error
} finally {
    Util.closeStream(is);
}

Not a lot else to do as the IOException in the catch might have some specific processing.

I like this, but I'd be inclined to go one step further, and create Util.openStream(url) so that callers aren't operating at 2 different levels of abstraction. — Carl Manaster, Oct 22 '10 at 14:33

Martín Schonaker · Answer 5 · 2010-10-22T05:47:13.203

You could define something like this somewhere:

private static interface InputStreamCallback {

    public void doIt(InputStream is) throws IOException;

}

private void with(InputStreamCallback cb) {

    InputStream is = null;

    // Creational code. Possibly adding an argument

    try {
        cb.doIt(is);
    } catch (IOException e) {
        // handle error or rethrow.
        // If rethrow add throws to method spec.
    } finally {
        if (is != null) {
            try {
                is.close();
            } catch (IOException e) {
                // handle error or rethrow.
            }
        }
    }
}

And invoke your code like this:

with(new InputStreamCallback() {

    @Override
    public void doIt(InputStream is) throws IOException {
        is = url.openConnection().getInputStream();
        // .....
    }

});

If you declare with method static in a helper class, then you could even do an import static of it.

There's a drawback. You need to declare url final.

EDIT: creational code is not the point. You can arrange it in several ways. The callback is the point. You could isolate what you need to do there.

How do I refactor closing a stream in Java?

5 Answers5