different try-finally approach

[Mike Schilling]

In actual practice I tend to have a utility method like this somewhere
in a project that uses lots of I/O:

public static void close (Closeable c) {
if (c != null) {
try {
c.close();
} catch (IOException e) {
// Ignore.
}
}
}

which gets rid of the (IMHO useless) close() exceptions, and also
allows nulls (which it ignores).

The exceptions are pretty useless if you're done reading a stream. If
you're done writing one, they might be telling you that output wasn't
flushed correctly, which is a bad thing to ignore.

 

[Bill McCleary]

The exceptions are pretty useless if you're done reading a stream. If
you're done writing one, they might be telling you that output wasn't
flushed correctly, which is a bad thing to ignore.

Explicitly calling flush() on an OutputStream at the end of the try
block should suffice to take care of that, shouldn't it? If it's gonna
throw while flushing, it will then throw in the try block.

 

[Lew]

In both situations constructor is allowed to fail. But in "java [sic] way" [sic] if
constructor fails, the close() statement is never called, so it's pretty
much the same thing, right?

Neither way is a particularly "Java" way.

I guess the bottom line is wheather you'll use internal catch or an
external one. I prefer the external ones, or even dividing the code
between methods.

I prefer:

public void doSomething()
// throws [MyResource|App]Exception
{
final MyResource res;
try
{
res = new MyResource();
}
catch ( MyResourceException exc )
{
logger.error( exc );
return; // or throw exc or new AppException(exc)
}
assert res != null;

try
{
// do something with res
}
finally
{
res.close()
}
}

 

[Mike Schilling]

Explicitly calling flush() on an OutputStream at the end of the try
block should suffice to take care of that, shouldn't it? If it's
gonna
throw while flushing, it will then throw in the try block.

Probably. If you're writing to a file, I wouldn't guarantee that the
directory entry is fully updated until the file is closed.

If you're going to write helper methods to simplify this, why not
something like (neither compiled nor tested, and I wouldn't vouch for
"thought through")

public static void close(Closeable... c) throws IOException
{
List<IOException> exceptions = new ArrayList<IOException>();
for (Closeable cl : c)
{
try { cl.close()
} catch (IOException ex) {
exceptions.add(ex) }
}
if (exceptions.size() > 0)
throw new WrappedIOException(exceptions);
}

 

[Lew]

Now, look at this one:

public void copyFiles(String inFile, String outFile)
throws IOException
{
FileInputStream inStream = new FileInputStream(inFile);
try
{
copyToFile(inStream, outFile);
}
finally
{
inStram.close();
}
}

private void copyToFile(InputStream inStream, String outFile)
throws IOException
{
FileOutputStream outStream = new FileOutputStream(inFile);
try
{
// copying code...
}
finally
{
outStream.close();
}
}

Eh. This approach breaks up the logic so it's harder to see that the input
and output streams are tightly coupled. It adds lines of source code without
appreciably improving clarity, perhaps even going the other way. It puts
streams that are at the same logic level in the algorithm into different
subroutine levels in the code. It factors the algorithm in a conceptually
jarring way.

I'd go with:

public void copy( String in, String out )
throws IOException
{
final InputStream is;
final OutputStream os;

try
{
is = new FileInputStream( in );
}
catch ( IOException exc )
{
logger.error( "Cannot open input stream", exc );
throw exc;
}

try
{
os = new FileOutputStream( out );
}
catch ( IOException exc )
{
logger.error( "Cannot open output stream", exc );
close( is );
throw exc;
}
assert is != null && os != null;

try
{
copy( is, os );
}
finally
{
close( os );
close( is );
}
}

 

[Bill McCleary]

public static void close(Closeable... c) throws IOException
{
List<IOException> exceptions = new ArrayList<IOException>();
for (Closeable cl : c)
{
try { cl.close()
} catch (IOException ex) {
exceptions.add(ex) }
}
if (exceptions.size() > 0)
throw new WrappedIOException(exceptions);
}

Cute.

Why not

switch (exceptions.size()) {
case 0: return;
case 1: throw exceptions.get(0);
case 2: throw new WrappedIOException(exceptions);
}

 

[Lew]

Ok, I'm in programming business like 20 years. I've worked with try-
blocks in Delphi and C#. I'm working with java for about 6 years now.
I've always used the style I've learned from Delphi an C#, which is
without the null assigments.

Now, I've seen hundreds of examples with the null assigment. All my
colleagues in several companies used that style. You actually cannot
find a different example on the whole Sun's site!!

So I was wondering does Sun knows something that I don't??

Perhaps it has something to do with multithreading? Deadlocks??

Nah, just simple-mindedness.

 

[Mike Schilling]

Cute.

Why not

switch (exceptions.size()) {
case 0: return;
case 1: throw exceptions.get(0);
case 2: throw new WrappedIOException(exceptions);
}

Cool. And you can also make the allocation of "exceptions" lazy if
that kind of optimization is important to you.

 

[Bill McCleary]

Eh. This approach breaks up the logic so it's harder to see that the
input and output streams are tightly coupled. It adds lines of source
code without appreciably improving clarity, perhaps even going the other
way. It puts streams that are at the same logic level in the algorithm
into different subroutine levels in the code. It factors the algorithm
in a conceptually jarring way.

Except, of course, if the same program downloads files from the web or
otherwise obtains them. And then having the copying logic in just one
place, to save to a file from an arbitrary input stream, becomes a big win.

 

[Bill McCleary]

Cool. And you can also make the allocation of "exceptions" lazy if
that kind of optimization is important to you.

What? One teensy little ArrayList in what's invariably going to be I/O
bound code?

 

[Mike Schilling]

What? One teensy little ArrayList in what's invariably going to be
I/O
bound code?

That's my feeling too, but at some point, someone anal is going to
profile it and ask where all those empty ArrayLists came from

 

[Mike Schilling]

Except, of course, if the same program downloads files from the web
or
otherwise obtains them. And then having the copying logic in just
one
place, to save to a file from an arbitrary input stream, becomes a
big win.

Agreed, though they shouldn't be private methods; they should be
public static methods in a common utility class.

 

[Pitch]

In both situations constructor is allowed to fail. But in "java [sic] way" [sic] if
constructor fails, the close() statement is never called, so it's pretty
much the same thing, right?

Neither way is a particularly "Java" way.

I guess the bottom line is wheather you'll use internal catch or an
external one. I prefer the external ones, or even dividing the code
between methods.

I prefer:

public void doSomething()
// throws [MyResource|App]Exception
{
final MyResource res;
try
{
res = new MyResource();
}
catch ( MyResourceException exc )
{
logger.error( exc );
return; // or throw exc or new AppException(exc)
}
assert res != null;

try
{
// do something with res
}
finally
{
res.close()
}
}

This implies res is not going to get null in the last try-block. I think
this is a real possibility if more programmers work on the code so you'd
still need if != null

 

[Pitch]

Eh. This approach breaks up the logic so it's harder to see that the input
and output streams are tightly coupled.

But I think they are not coupled at all.

Maybe in terms of business logic you can say that, but on the technical
level what you have is acccessing one resource, and than accessing
another, both with their on set of rules and exceptions.

We are now talking about input/output streams in java, but that could
easily become a COM-wrapper, db-connection, specific API that requires
clean-up code etc..

 

[Stefan Ram]

Ok. I'd like to think readability is more important than performance.

This statements seems somewhat odd to me, because you seem
to insinuate that I had written »price with regard to
performance«. Actually, this makes me somewhat annoyed,
because I clearly did not write this. So this seem to be a

http://en.wikipedia.org/wiki/Straw_man

against me.

And what is the intention of the symbol »«? Do you want
to express that this idea seems funny to you, or that you
are just kidding? I am investing effort and time to
decipher this ambigous symbol »«, which only adds to my
annoyance, because you could have avoided this by using a
more explicit and unambiguous means to express whatever
you wanted to express with »«.

Additional methods have a price also with regard to
readability:

o A custom method can hide well-known standard
API-methods. The reader might have immediatly known
the meaning of the standard API-calls, but will have
to look up the meaning of your custom method.

o A custom method increases code size by adding
its interface and documentation, making it harder
to browse and maintain the code.

o The additional methods increase the number of
entities in the code (the code complexity), which
makes it harder to keep them in mind.

Here is some evidence:

o Basili and Perricone found that routine size was
inversely correlated with errors: as the size of
routines increased (up to 200 lines of code), the
number of errors per line of code decreased (Basili
and Perricone 1984).

o A 1986 study found that small routines (32 lines of
code or fewer) were not correlated with lower cost or
fault rate (Card, Church, and Agresti 1986; Card and
Glass 1990). The evidence suggested that larger
routines (65 lines of code or more) were cheaper to
develop per line of code.

http://www.c2.com/cgi/wiki?LongFunctions

o Routines with fewer than 143 source statements
(including comments) had 23% more errors per line of
code than larger routines. [Selby and Basili, 1991]

http://dubroy.com/blog/method-length-are-short-methods-actually-worse/

There is an optimum somewhere in between too large
and too small:

http://www.leshatton.org/Documents/Ubend_IS697.pdf

 

[Lew]

In both situations constructor is allowed to fail. But in "java [sic] way" [sic] if
constructor fails, the close() statement is never called, so it's pretty
much the same thing, right?

Neither way is a particularly "Java" way.

I guess the bottom line is wheather you'll use internal catch or an
external one. I prefer the external ones, or even dividing the code
between methods.

I prefer:

public void doSomething()
// throws [MyResource|App]Exception
{
final MyResource res;
try
{
res = new MyResource();
}
catch ( MyResourceException exc )
{
logger.error( exc );
return; // or throw exc or new AppException(exc)
}
assert res != null;

try
{
// do something with res
}
finally
{
res.close()
}
}

This implies res is not going to get null in the last try-block. I think
this is a real possibility if more programmers work on the code so you'd
still need if != null

No, 'res' is guaranteed not to be null.

 

[Pitch]

This statements seems somewhat odd to me, because you seem
to insinuate that I had written ?price with regard to
performance?. Actually, this makes me somewhat annoyed,
because I clearly did not write this. So this seem to be a

http://en.wikipedia.org/wiki/Straw_man

against me.

OK, so I got it wrong. You don't have to write two pages about it, just
say so. I'll apologize, put another two smiles, and life goes on.

And what is the intention of the symbol ??? Do you want
to express that this idea seems funny to you, or that you
are just kidding?

I wanted to say that I have an opsessive compulsive disorder about the
code-readability.

Additional methods have a price also with regard to
readability:

o A custom method can hide well-known standard
API-methods. The reader might have immediatly known
the meaning of the standard API-calls, but will have
to look up the meaning of your custom method.

The name of the method must explain what it is for. If you can't achive
that, your code is not designed well.

"safeCloseConnection" tells everything about the method. The word
"safe" may not be clear to everyone, but from the context of the code it
should be understandable.

o A custom method increases code size by adding
its interface and documentation, making it harder
to browse and maintain the code.

Wrong. You need to document things only if they don't comform to well-
known standards or company standards. In my company we decided that
every method which begins with "safe" must not throw _any_ exceptions.

o The additional methods increase the number of
entities in the code (the code complexity), which
makes it harder to keep them in mind.

You don't need to keep code entities in mind, you just read them. The
more readable, the more you understand.

One long method is hard to read, which raises the cost of code-
maintenance and is prone to errors. Method bodies should not be longer
than 15-20 lines. Of course, there are situations where you need to
write a block of long procedural code.

Here is some evidence:

o Basili and Perricone found that routine size was
inversely correlated with errors: as the size of
routines increased (up to 200 lines of code), the
number of errors per line of code decreased (Basili
and Perricone 1984).

o A 1986 study found that small routines (32 lines of
code or fewer) were not correlated with lower cost or
fault rate (Card, Church, and Agresti 1986; Card and
Glass 1990). The evidence suggested that larger
routines (65 lines of code or more) were cheaper to
develop per line of code.

o Routines with fewer than 143 source statements
(including comments) had 23% more errors per line of
code than larger routines. [Selby and Basili, 1991]

http://dubroy.com/blog/method-length-are-short-methods-actually-worse/

This is ridiculous. Size is not related to errors. Expirience is, and
coding-practices, rules etc..

 

[Pitch]

public void doSomething()
// throws [MyResource|App]Exception
{
final MyResource res;
try
{
res = new MyResource();
}
catch ( MyResourceException exc )
{
logger.error( exc );
return; // or throw exc or new AppException(exc)
}
assert res != null;

try
{
// do something with res
}
finally
{
res.close()
}
}

This implies res is not going to get null in the last try-block. I think
this is a real possibility if more programmers work on the code so you'd
still need if != null

No, 'res' is guaranteed not to be null.

What about this:

assert res != null;

try
{
// do something with res
res = null;
}
finally
{
res.close()
}

 

[Bill McCleary]

public void doSomething()
// throws [MyResource|App]Exception
{
final MyResource res; [snip]
try
{
// do something with res
}
finally
{
res.close()
}
}
This implies res is not going to get null in the last try-block. I think
this is a real possibility if more programmers work on the code so you'd
still need if != null

No, 'res' is guaranteed not to be null.

What about this:

assert res != null;

try
{
// do something with res
res = null;
}
finally
{
res.close()
}

Error: cannot assign a value to final variable res
1 error

 

[Pitch]

public void doSomething()
// throws [MyResource|App]Exception
{
final MyResource res; [snip]
try
{
// do something with res
}
finally
{
res.close()
}
}
This implies res is not going to get null in the last try-block. I think
this is a real possibility if more programmers work on the code so you'd
still need if != null
No, 'res' is guaranteed not to be null.

What about this:

assert res != null;

try
{
// do something with res
res = null;
}
finally
{
res.close()
}

Error: cannot assign a value to final variable res
1 error

oops, my fault