When is finally justified?

[pek]

If there is no return statement in neither try nor catch, is finally
any useful? And even then, doesn't finally simply help you avoid
duplicating the code? (Not that this is a bad thing per se, just
asking).

I recently wrote a blog post starting with the following text which I
think is not a good start:

"First and foremost, a finally clause has no reason to exist if there
isn’t a return statement inside the try or catch clause. And even
then, it helps you avoid duplicating code."

I would like some comments/reviews/recommendations etc.

Thank you.

 

[Lew]

If there is no return statement in neither try nor catch, is finally
any useful? And even then, doesn't finally simply help you avoid
duplicating the code? (Not that this is a bad thing per se, just
asking).

I recently wrote a blog post starting with the following text which I
think is not a good start:

"First and foremost, a finally clause has no reason to exist if there
isn’t a return statement inside the try or catch clause. And even
then, it helps you avoid duplicating code."

Confusing and inaccurate.

'finally' is for when you want to ensure that something happens after
a block of code, and is used in all sorts of scenarios not involving
'return'. One significant use case is for the release of resources, a
poor-man's "Resource Acquisition is Initialization (RAII)".
<http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization>

Duplication of code is not a major issue requiring 'finally' - it is
code safety, guaranteeing the release of resources, and the prevention
of exceptions that drive its use. Your "first and foremost" claim is
incorrect and will mislead budding programmers who don't know better.

I would like some comments/reviews/recommendations etc.

Study what 'finally' is really for before you go spreading false ideas
about it.

 

[pek]

Confusing and inaccurate.

'finally' is for when you want to ensure that something happens after
a block of code, and is used in all sorts of scenarios not involving
'return'.  One significant use case is for the release of resources, a
poor-man's "Resource Acquisition is Initialization (RAII)".
<http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization>

Duplication of code is not a major issue requiring 'finally' - it is
code safety, guaranteeing the release of resources, and the prevention
of exceptions that drive its use.  Your "first and foremost" claim is
incorrect and will mislead budding programmers who don't know better.


Study what 'finally' is really for before you go spreading false ideas
about it.

Well, I still have the intro as draft and wanted to hear some
comments. It's indeed inaccurate, but my intention was not to explain
what does finally do but to talk about it in the context of returning
inside it.

Your comments where really helpful. Thank you. I'll try to use them.

 

[pek]

But I can avoid using a finally block and still accomplish the same
goal. For example, this code :

public String readString() {
String oneLine;
try {
FileReader theFile = new FileReader( fileName );
BufferedReader fileIn = new BufferedReader( theFile );
oneLine = fileIn.readLine();
} catch (Exception e) {
// Oversimplified catch clause
}
fileIn.close();

return oneLine;
}

and this code:

public String readString() {
String oneLine;
try {
FileReader theFile = new FileReader( fileName );
BufferedReader fileIn = new BufferedReader( theFile );
oneLine = fileIn.readLine();
fileIn.close();
return oneLine;
} catch (Exception e) {
// Oversimplified catch clause
fileIn.close();
return oneLine;
}
}

is the same as this:

public String readString() {
String oneLine;
try {
FileReader theFile = new FileReader( fileName );
BufferedReader fileIn = new BufferedReader( theFile );
oneLine = fileIn.readLine();
return oneLine;
} catch (Exception e) {
// Oversimplified catch clause
return oneLine;
} finally {
fileIn.close();
}
}

 

[pek]

Indeed, I forgot to mention that the article was in the scope of
having a return statement somewhere (try or catch or finally block). I
kinda changed that but I think I now have another thought.

I can completely remove the finally block in your example and the code
works exactly the same. I also think that I can completely remove the
finally as a general rule in which case I have the drawbacks I mention
previously: duplication of code and returning outside a try/catch.

Of course I think this is a bad programming practice and wouldn't
actually do that. I'm just trying to figure out things.

As for the resource example, I simply have to close the opened
resource. It doesn't have to be inside a finally. Finally simplifies
this process.

Any comments on this?

Note: I'm brainstorming hear to get some opinions and thoughts.

 

[Lew]

Please do not top-post.

But I can avoid using a finally block and still accomplish the same
goal. For example, this code :

public String readString() {
    String oneLine;
    try {
        FileReader theFile = new FileReader( fileName );
        BufferedReader fileIn  = new BufferedReader( theFile );
        oneLine = fileIn.readLine();
    } catch (Exception e) {
        // Oversimplified catch clause
    }
    fileIn.close();

    return oneLine;

}

and this code:

public String readString() {
    String oneLine;
    try {
        FileReader theFile = new FileReader( fileName );
        BufferedReader fileIn  = new BufferedReader( theFile );
        oneLine = fileIn.readLine();
        fileIn.close();
        return oneLine;
    } catch (Exception e) {
        // Oversimplified catch clause
        fileIn.close();
        return oneLine;
    }

}

is the same as this:

public String readString() {
    String oneLine;
    try {
        FileReader theFile = new FileReader( fileName );
        BufferedReader fileIn  = new BufferedReader( theFile );
        oneLine = fileIn.readLine();
        return oneLine;
    } catch (Exception e) {
        // Oversimplified catch clause
        return oneLine;
    } finally {
        fileIn.close();
    }

}

In many situations such as the simplified cases you described there
are often many ways to accomplish the same thing. 'for' and 'while'
are roughly equivalent, except that 'for' allows tighter control of
variable scope for a loop variable. Likewise, the idioms you show are
equivalent except for when you don't fall through from the exception.

Consider a SQL query where you need to establish a connection, build a
statement and execute it. Each step depends on the previous one, but
if the connection succeeds you absolutely must close it. A simplified
(i.e., non-compilable) example is:

Collection <Foo> coll = new ArrayList <Foo> ();

Connection cxn;
try
{
cxn = DriverManager.getConnection(...);
}
catch ( SQLException exc )
{
logger.error( exc );
return null;
}
assert cxn != null;

PreparedStatement ps;
try
{
ResultSet rs;

ps = cxn.prepareStatement( QUERY );

assert ps != null;
try
{
rs = ps.executeQuery()
assert rs != null;

if ( rs.next() )
{
Foo foo;
... // extract ResultSet into foo;
coll.add( foo );
}
rs.close();
}
catch ( SQLException exc )
{
logger.error( exc );
}
finally
{
ps.close();
}

ps = cxn.prepareStatement( ANOTHER_QUERY );

assert ps != null;
try
{
rs = ps.executeQuery()
assert rs != null;

if ( rs.next() )
{
Foo foo;
... // extract ResultSet into foo;
coll.add( foo );
}
rs.close();
}
catch ( SQLException exc )
{
logger.error( exc );
}
finally
{
ps.close();
}
}
catch ( SQLException exc )
{
logger.error( exc );
}
finally
{
try
{
cxn.close();
}
catch ( SQLException exc )
{
logger.error( exc );
}
}
return coll;

 

[pek]

Correct. I could remove the finally though if I had a catch block
right? For example, the previous code would be:

Writer out = ...;
try {
...
out.close();
}
catch (Exception e) {
out.close();
}

Indeed, I forgot to mention that the article was in the scope of
having a return statement somewhere (try or catch or finally block). I
kinda changed that but I think I now have another thought.

I can completely remove the finally block in your example and the code
works exactly the same. [...]

No, you can't.  If an exception is thrown, the code that was in the  
finally clause you removed would not execute.

[...]
As for the resource example, I simply have to close the opened
resource. It doesn't have to be inside a finally. Finally simplifies
this process.

It does more than just simplify it.  It allow you to have a single block  
of code that is executed whether an exception happens or not, and whether  
or not you catch the exception.

Pete

 

[Lothar Kimmeringer]

But I can avoid using a finally block and still accomplish the same
goal. For example, this code :

public String readString() {
String oneLine;
try {
FileReader theFile = new FileReader( fileName );
BufferedReader fileIn = new BufferedReader( theFile );
oneLine = fileIn.readLine();
} catch (Exception e) {
// Oversimplified catch clause
}
fileIn.close();

return oneLine;
}

This will not compile, because of the scope of fileIn,
because oneLine might not be initialized and because
BufferedReader.close() can throw an IOException. And even
If you really want to e.g. return null in case of an exception
instead of passing the exception you are not making sure that
the resource is closed all the time. If an Error instead of
an Exception occures (e.g. an OutOfMemoryError), you won't
be inside the catch-clause leaving the resource unclosed
until the garbage collector comes around,

In general you see finally in code-blocks like this:

public String readLine() throws IOException{

Reader fileIn = null;
try{
FileReader fr = new FileReader(fileName);
fileIn = new BufferedReader(InputStreamReader(fr, "8859_1"));
return fileIn.readLine();
}
finally{
if (fileIn != null){
try{
fileIn.close();
catch(Exception e){}
}
}

}

So the method can pass an occured IOException but still
makes sure that all resources are closed. If you want
to do this without the use of finally you would have
to do something like this:

try{
...
}
catch(Throwable t){
if (fileIn != null){
try{
fileIn.close();
}
catch(Exception e){}
}
if (t instanceof ThreadDeath){
throw (ThreadDeath) t;
}
if (t instanceof RuntimeException){
throw (RuntimeException) t;
}
if (t instanceof IOException){
throw (IOException) t;
}
throw (IOException) new IOException("an exception occured while " +
"accessing the resource").initCause(t);
}

Quite unreadable, isn't it?

and this code:

public String readString() {
String oneLine;
try {
FileReader theFile = new FileReader( fileName );
BufferedReader fileIn = new BufferedReader( theFile );
oneLine = fileIn.readLine();
fileIn.close();
return oneLine;
} catch (Exception e) {
// Oversimplified catch clause
fileIn.close();
return oneLine;
}
}

This is actually the same, i.e. it won't compile and duplicates
code violating the DRY-principle.

is the same as this:

public String readString() {
String oneLine;
try {
FileReader theFile = new FileReader( fileName );
BufferedReader fileIn = new BufferedReader( theFile );
oneLine = fileIn.readLine();
return oneLine;
} catch (Exception e) {
// Oversimplified catch clause
return oneLine;
} finally {
fileIn.close();
}
}

This is no the same, because it won't compile for the additional
reason, that the method doesn't return a value all the time.


Regards, Lothar
--
Lothar Kimmeringer E-Mail: (e-mail address removed)
PGP-encrypted mails preferred (Key-ID: 0x8BC3CD81)

Always remember: The answer is forty-two, there can only be wrong
questions!

 

[Lew]

[...]
and this code:

public String readString() {
String oneLine;
try {
FileReader theFile = new FileReader( fileName );
BufferedReader fileIn = new BufferedReader( theFile );
oneLine = fileIn.readLine();
fileIn.close();
return oneLine;
} catch (Exception e) {
// Oversimplified catch clause
fileIn.close();
return oneLine;
}
}

... which won't compile ...

Even were it compilable, the 'return' lines might never execute.
<http://java.sun.com/javase/6/docs/api/java/io/BufferedReader.html#close()>

 

[Lew]

...

The method readString cannot complete normally, and every return
statement returns an expression of suitable type, so I don't see any
return problem.

It is possible that 'oneLine' might never be assigned a value as of the
'return' within the 'catch' block. It needs to be definitely assigned before
being returned.

'fileIn' is not in scope in the 'finally' block.

 

[Arne Vajhøj]

[...]
and this code:

public String readString() {
String oneLine;
try {
FileReader theFile = new FileReader( fileName );
BufferedReader fileIn = new BufferedReader( theFile );
oneLine = fileIn.readLine();
fileIn.close();
return oneLine;
} catch (Exception e) {
// Oversimplified catch clause
fileIn.close();
return oneLine;
}
}

... which won't compile ...

Even were it compilable, the 'return' lines might never execute.
<http://java.sun.com/javase/6/docs/api/java/io/BufferedReader.html#close()>

Even if it reaches the last return oneLine, then oneLine could have
a value or be null (assuming that is the value it is set to) in case
of an exception depending on where the exception happens - I don't
like that behavior.

Arne

 

[Lew]

pek wrote:
[...]
and this code:

public String readString() {
String oneLine;
try {
FileReader theFile = new FileReader( fileName );
BufferedReader fileIn = new BufferedReader( theFile );
oneLine = fileIn.readLine();
fileIn.close();
return oneLine;
} catch (Exception e) {
// Oversimplified catch clause
fileIn.close();
return oneLine;
}
}

... which won't compile ...

Even were it compilable, the 'return' lines might never execute.
<http://java.sun.com/javase/6/docs/api/java/io/BufferedReader.html#close()>

Even if it reaches the last return oneLine, then oneLine could have
a value or be null (assuming that is the value it is set to) in case
of an exception depending on where the exception happens - I don't
like that behavior.

All this uncompilable code - scope errors, variables not definitely assigned -
points up the need to use SSCCEs in this discussion.

 

[Arne Vajhøj]

If there is no return statement in neither try nor catch, is finally
any useful? And even then, doesn't finally simply help you avoid
duplicating the code? (Not that this is a bad thing per se, just
asking).

I recently wrote a blog post starting with the following text which I
think is not a good start:

"First and foremost, a finally clause has no reason to exist if there
isn’t a return statement inside the try or catch clause. And even
then, it helps you avoid duplicating code."

Completely wrong.

It is not even common to have a return statement where finally
is used.

You use a finally clause whenever you have some code that
you need executed with normal flow, with an exception, with
multiple exceptions etc..

It is typical used to release resources like open files
or open database connections.

Having a return statement in the try block with code being
executed in a finally block is in my opinion an anti-pattern,
that should not be used.

Arne

 

[pek]

So you're using USENET to discuss something
which you'll then blog?

Doesn't that make the blog redundant?

   BugBear

OK... Because things got way out of scope:

The reason why there where so many compile errors was because they
where irrelevant. It was like you where complaining about spelling
errors. I believe most of you understood that the code was an example
and with little rewriting you could correct the compile errors and
answer my question. i.e.:

public String readString(String fileName) {
String oneLine = null;
BufferedReader fileIn = null;
try {
FileReader theFile = new FileReader( fileName );
fileIn = new BufferedReader( theFile );
oneLine = fileIn.readLine();
return oneLine;
} catch (Exception e) {
// Oversimplified catch clause
return oneLine;
} finally {
try {
fileIn.close();
} catch (IOException e) {
// Do something
}
}
}

Furthermore, I totally agree with everyone that what I wrote about the
finally was completely wrong, misguided or plain stupid. The whole
idea about the blog post was to focus on how Java executes a try/catch/
finally block when there is a return statement somewhere around. I
found this interesting article and wanted to talk a little about it:
http://weblogs.java.net/blog/staufferjames/archive/2007/06/_dont_return_in.html

Also, I'm not, by any means, against the use of finally! I never said
that finally clause is evil. Of course I use it.

Finally, the reason why I posted this in the usenet was to get some
ideas and feedback. I love this community and I believe it is filled
with great minds. I posted that awful intro without thinking about it
because I was rushed to get to my point. After realizing that it was
indeed bad, I turned here. I failed to ask the right question and got
a bunch of rants about bad coding and compile errors.

I now completely erased that paragraph and paraphrased what Lew said
in his first reply.

Thank you all.

 

[Lew]

The reason why there where so many compile errors was because they
where irrelevant. It was like you where complaining about spelling
errors. I believe most of you understood that the code was an example
and with little rewriting you could correct the compile errors and
answer my question. i.e.:

I must respectfully disagree here. Your points needed to have
compilable code, at least in principle, in order to support the
claims. It is not like spelling errors, because the very purpose of
'finally' is illustrated by the validity or non-validity of the
alternative idioms that were presented. The compilation errors
actually invalidated the thesis that 'finally' wasn't needed. Also,
as a matter of courtesy one should not rely on those from whom you are
requesting assistance to fix preventable errors for you.

Regarding your new example:

public String readString(String fileName) {
  String oneLine = null;
  BufferedReader fileIn = null;
  try {
    FileReader theFile = new FileReader( fileName );
    fileIn  = new BufferedReader( theFile );
    oneLine = fileIn.readLine();
    return oneLine;
  } catch (Exception e) {

It is better to follow the advice given upthread and restrict the
'catch' to specific exceptions, in this case 'IOException'.

    // Oversimplified catch clause
    return oneLine;

It would be better to keep the scope 'oneLine' within the 'try' block,
and return a literal 'null' here.

  } finally {
    try {
      fileIn.close();

This line can be reached without 'fileIn' having been opened.

    } catch (IOException e) {
      // Do something
    }
  }

}

Here's how I suggest organizing this snippet:

<snippet>
BufferedReader in;
try
{
in = new BufferedReader( new FileReader( file ));
}
catch (IOException exc )
{
logger.error( exc ); // or whatever you do for logging
return null; // or throw custom or run-time exception
}
assert in != null; // this is an invariant - 'assert' is good

try
{
String oneLine = in.readLine();
return oneLine;
}
catch ( IOException exc )
{
logger.error( exc ); // or whatever you do for logging
return null; // or throw custom or run-time exception
}
finally
{
try
{
in.close();
}
catch ( IOException exc )
{
logger.error( exc ); // or whatever you do for logging
}
}
</snippet>

Notice that scope is minimized for 'oneLine', and that 'in' isn't
closed unless it has been opened.

 

[Arne Vajhøj]

The reason why there where so many compile errors was because they
where irrelevant. It was like you where complaining about spelling
errors. I believe most of you understood that the code was an example
and with little rewriting you could correct the compile errors and
answer my question.

We could.

But I think it is disrespectful to ask us to do it.

You want our help with something. Then it makes sense
that you spend the few minutes it takes to make the
code compilable instead of leaving it to us.

As a bonus you will get focus on your real problem instead
of having a dozen posts discussing the compilation errors.

Arne

 

[Lothar Kimmeringer]

The method readString cannot complete normally, and every return
statement returns an expression of suitable type, so I don't see any
return problem.

I overlooked the return in the catch-part.


Regards, Lothar
--
Lothar Kimmeringer E-Mail: (e-mail address removed)
PGP-encrypted mails preferred (Key-ID: 0x8BC3CD81)

Always remember: The answer is forty-two, there can only be wrong
questions!

 

[Lew]

I overlooked the return in the catch-part.

In which, as presented, the variable 'oneLine' has not been definitely assigned.

 

[Lothar Kimmeringer]

In which, as presented, the variable 'oneLine' has not been
definitely assigned.

This hasn't changed to the code-snippets I commented before,
the "new feature" was just possible without the return-statement
in the catch-block.


Regards, Lothar
--
Lothar Kimmeringer E-Mail: (e-mail address removed)
PGP-encrypted mails preferred (Key-ID: 0x8BC3CD81)

Always remember: The answer is forty-two, there can only be wrong
questions!

 

[Roger Lindsjö]

I can completely remove the finally block in your example and the code
works exactly the same. I also think that I can completely remove the
finally as a general rule in which case I have the drawbacks I mention
previously: duplication of code and returning outside a try/catch.

Simple example with finally:
public class Thrower {
public static void main(String[] args) {
new Thrower().callit();
System.err.println("I better NOT be printed");
}

private void callit() {
try {
doA();
} finally {
doB();
}
}

private void doB() {
System.err.println("I better be printed");
}

private void doA() {
throw new RuntimeException("Did not expect me, did you?");
}
}

To get the same behaviour without finally I'd need soething like:
public class Thrower2 {
public static void main(String[] args) {
new Thrower2().callit();
System.err.println("I better NOT be printed");
}

private void callit() {
try {
doA();
} catch (Throwable t) {
doB();
T.local.set(t);
try {
T.class.newInstance();
} catch (InstantiationException e) { // Should not happen
} catch (IllegalAccessException e) { // Should not happen
}
}
}

private void doB() {
System.err.println("I better be printed");
}

private void doA() {
throw new RuntimeException("Did not expect me, did you?");
}

private static class T {
static ThreadLocal<Throwable> local = new ThreadLocal<Throwable>();
public T() throws Throwable {
throw local.get();
}
}
}

And even then I have introduced the chance that T is not available (see
the two exception I have chosen to ignore).