finally block does not complete normally - Eclipse

[dannyDog]

I've started working on a new project using Eclipse. When I do a build
I get multiple warnings stating "finally block does not complete
normally".

A sample method follows:
/**
* Property getter for destination type
* @return type
*/
public String getDestinationType()
{
String destType = "SCHOOL_DISTRICT";
try
{
destType = this.getReceiving().getDestinationType();
if (destType.length() == 0)
{
destType = "SCHOOL_DISTRICT";
}
}
catch (Exception e)
{
destType = "SCHOOL_DISTRICT";
}
finally
{
return destType;
}
}

This type of coding of returning the return value in the finally block
is in many, many, many methods over many, many, many classes.

What is the correct way to code try/catch/finally return value?

 

[shakah]

I've started working on a new project using Eclipse. When I do a build
I get multiple warnings stating "finally block does not complete
normally".

A sample method follows:
/**
* Property getter for destination type
* @return type
*/
public String getDestinationType()
{
String destType = "SCHOOL_DISTRICT";
try
{
destType = this.getReceiving().getDestinationType();
if (destType.length() == 0)
{
destType = "SCHOOL_DISTRICT";
}
}
catch (Exception e)
{
destType = "SCHOOL_DISTRICT";
}
finally
{
return destType;
}
}

This type of coding of returning the return value in the finally block
is in many, many, many methods over many, many, many classes.

What is the correct way to code try/catch/finally return value?

Well, without commenting on the coding style, you could simply
eliminate the finally block, i.e.:

public String getDestinationType() {
String destType = "SCHOOL_DISTRICT";
try {
destType = this.getReceiving().getDestinationType();
if (destType.length() == 0) {
destType = "SCHOOL_DISTRICT";
}
}
catch (Exception e) {
destType = "SCHOOL_DISTRICT";
}

return destType;
}

 

[JaMess]

I've started working on a new project using Eclipse. When I do a build
I get multiple warnings stating "finally block does not complete
normally".

A sample method follows:
/**
* Property getter for destination type
* @return type
*/
public String getDestinationType()
{
String destType = "SCHOOL_DISTRICT";
try
{
destType = this.getReceiving().getDestinationType();
if (destType.length() == 0)
{
destType = "SCHOOL_DISTRICT";
}
}
catch (Exception e)
{
destType = "SCHOOL_DISTRICT";
}
finally
{
return destType;
}
}

This type of coding of returning the return value in the finally block
is in many, many, many methods over many, many, many classes.

What is the correct way to code try/catch/finally return value?

break, continue return and throw are abrupt completions of statements.
You shoud do the return outside the "finally" block.
Something like this:
/**
* Property getter for destination type
* @return type
*/
public String getDestinationType()
{
String destType = "SCHOOL_DISTRICT";
try
{
destType = this.getReceiving().getDestinationType();
if (destType.length() == 0)
{
destType = "SCHOOL_DISTRICT";
}
}
catch (Exception e)
{
destType = "SCHOOL_DISTRICT";
}
finally
{
//some code that NEEDS to be executed no matter what, such as
//closing a connection to a database or other stuff that
//should happen always.
//otherwise don't use finally block.
}
return destType;
}

Anyway, I would write it like this:

/**
* Property getter for destination type
* @return type
*/
public String getDestinationType() {
String destType = null;
try {
destType = this.getReceiving().getDestinationType();
}catch (Exception e){}
if(destType == null || destType.length() == 0) {
destType = "SCHOOL_DISTRICT";
}
return destType;
}

 

[Erik Frohne]

Anyway, I would write it like this:

/**
* Property getter for destination type
* @return type
*/
public String getDestinationType() {
String destType = null;
try {
destType = this.getReceiving().getDestinationType();
}catch (Exception e){}
if(destType == null || destType.length() == 0) {
destType = "SCHOOL_DISTRICT";
}
return destType;
}

A general exception thrown has usually a reason behind it.
At least you should do something to inform the user, that
something bad did just happen. The snippet

catch( Exception e ) {
e.printStackTrace();
}

is the minimum I would do here. Anyway, if the called code
could throw a specific (and thus a wanted) exception (such
as IOException) I would try to catch it.

Just my 2 cents.


Erik.

 

[Patricia Shanahan]

I've started working on a new project using Eclipse. When I do a build
I get multiple warnings stating "finally block does not complete
normally".

A sample method follows:
/**
* Property getter for destination type
* @return type
*/
public String getDestinationType()
{
String destType = "SCHOOL_DISTRICT";
try
{
destType = this.getReceiving().getDestinationType();
if (destType.length() == 0)
{
destType = "SCHOOL_DISTRICT";
}
}
catch (Exception e)
{
destType = "SCHOOL_DISTRICT";
}
finally
{
return destType;
}
}

This type of coding of returning the return value in the finally block
is in many, many, many methods over many, many, many classes.

What is the correct way to code try/catch/finally return value?

The correct way depends on what you want to have happen.

In particular, how do you want to handle Error?

Patricia

 

[Roland]

I've started working on a new project using Eclipse. When I do a build
I get multiple warnings stating "finally block does not complete
normally".

A sample method follows:
/**
* Property getter for destination type
* @return type
*/
public String getDestinationType()
{
String destType = "SCHOOL_DISTRICT";
try
{
destType = this.getReceiving().getDestinationType();
if (destType.length() == 0)
{
destType = "SCHOOL_DISTRICT";
}
}
catch (Exception e)
{
destType = "SCHOOL_DISTRICT";
}
finally
{
return destType;
}
}

This type of coding of returning the return value in the finally block
is in many, many, many methods over many, many, many classes.

What is the correct way to code try/catch/finally return value?

A return in a finally block is a bad style of coding.
Simply because it "eats" all exceptions and errors that get thrown.

Consider the following example. When you run it, you miss the
OutOfMemoryError, because this finally block causes the method to return
as if nothing happened.

public class ReturnInFinally {
private static String getDestinationType() {
String destType = "SCHOOL_DISTRICT";
try {
destType = "Explicitly Set";
throw new OutOfMemoryError();
} finally {
return destType;
}
}

public static void main(String[] args) {
try {
System.out.println(getDestinationType());
} catch (Throwable t) {
t.printStackTrace();
}
}
}


I guess you miss several exceptions and errors in many, many, many parts
of your code. I think you should take the time and effort to change them
all. [FWIW, I've set Eclipse to flag "finally block does not complete
normally" as an *error* rather than a warning.]
--
Regards,

Roland de Ruiter
___ ___
/__/ w_/ /__/
/ \ /_/ / \

 

[Mike Schilling]

I've started working on a new project using Eclipse. When I do a build
I get multiple warnings stating "finally block does not complete
normally".

A sample method follows:
/**
* Property getter for destination type
* @return type
*/
public String getDestinationType()
{
String destType = "SCHOOL_DISTRICT";
try
{
destType = this.getReceiving().getDestinationType();
if (destType.length() == 0)
{
destType = "SCHOOL_DISTRICT";
}
}
catch (Exception e)
{
destType = "SCHOOL_DISTRICT";
}
finally
{
return destType;
}
}

This type of coding of returning the return value in the finally block
is in many, many, many methods over many, many, many classes.

What is the correct way to code try/catch/finally return value?

A return in a finally block is a bad style of coding.
Simply because it "eats" all exceptions and errors that get thrown.

Consider the following example. When you run it, you miss the
OutOfMemoryError, because this finally block causes the method to return
as if nothing happened.

public class ReturnInFinally {
private static String getDestinationType() {
String destType = "SCHOOL_DISTRICT";
try {
destType = "Explicitly Set";
throw new OutOfMemoryError();
} finally {
return destType;
}
}

public static void main(String[] args) {
try {
System.out.println(getDestinationType());
} catch (Throwable t) {
t.printStackTrace();
}
}
}


I guess you miss several exceptions and errors in many, many, many parts
of your code. I think you should take the time and effort to change them
all. [FWIW, I've set Eclipse to flag "finally block does not complete
normally" as an *error* rather than a warning.]

FWIW, C# does not allow returning from a finally block.

 

[enrique]

So far no one has commented on _why_ one would use a finally statement,
so I guess I will. Maybe it will help you understand why you're code
is behaving the way you expect.

"finally" tells the JVM to execute the enclosed code in the event of
any exception. No matter how bad things get, I believe code within the
scope of the finally statement is guaranteed to execute -- no matter
what.

So the way you have your code shown, the "return" statement happens
_only_ upon an exception. If there is no exception, it doesn't execute.

 

[Mike Schilling]

So far no one has commented on _why_ one would use a finally statement,
so I guess I will. Maybe it will help you understand why you're code
is behaving the way you expect.

"finally" tells the JVM to execute the enclosed code in the event of
any exception. No matter how bad things get, I believe code within the
scope of the finally statement is guaranteed to execute -- no matter
what.

Yes, that's true -- exception or not.

So the way you have your code shown, the "return" statement happens
_only_ upon an exception. If there is no exception, it doesn't execute.

No, it always executes..

The classic use for finally is to clean up resources, for intance open
files, e.g.

FileOuputStream fos = null;

try {
fos = new FileOutputStream(FILENAME);
... // write to file
} finally {
if (fos != null)
fos.close();
}

Regardless of what happens inside the try block, the file gets closed.

 

[Antti S. Brax]

The classic use for finally is to clean up resources, for intance open
files, e.g.

FileOuputStream fos = null;

try {
fos = new FileOutputStream(FILENAME);
... // write to file
} finally {
if (fos != null)
fos.close();
}

Please don't forget to put a try-catch block inside the finally
block (this is very important in example code because people use
it to learn things).

 

[Chris Smith]

Please don't forget to put a try-catch block inside the finally
block (this is very important in example code because people use
it to learn things).

I strongly disagree that it's a good idea to put a try/catch inside the
finally.

After all, what do you intend to do with the exception when you catch
it? Are you saying that the code that reads the file ought to have
knowledge of the exception handling strategy for the software?

Chris's First Law of Exception Handling:

"Exception handling should reduce coupling, not increase it."

Corollary:

"If you don't know what to do with an exception, then don't
try to handle it. You're better off leaving it to someone
else."

Too often, people taking this kind of advice -- to always handle
exceptions -- do something ineffective, like just write it to standard
output then drop it. The problem is that if finally executes because
the original code completed normally, then you miss proper handling of
the exception and it very likely gets forgotten. That leads to
undetected faults in cases like running out of disk space while flushing
the buffers at the end of a write operation.

Chris's Second Law of Exception Handling:

"No one ever reads standard output, even if they should."

Yes, this does mean that if an exception is thrown by the code that
writes to the file, and again by code that closes a file, then the first
of the two exceptions will be lost. This is probably the reason for
your recommendation to catch exceptions in finally. Generally speaking,
though, the two exceptions will be caused by EXACTLY the same thing, and
having just one of the two exceptions for debugging is good enough.

Chris's Third Law of Exception Handling:

"The MOST important concern in exception handling is to be sure
that the existence of a problem is recognized. What information
you get about the problem is only a secondary concern."

So I see nothing wrong with the finally block,a nd would object to
catching exceptions there. You'd be potentially sacrificing even
finding out about some problems, in order to obtain marginally better
information about problems that you already know.

That said, I would change one thing, so the code would read:

FileOuputStream fos = new FileOutputStream(FILENAME);

try
{
... // write to file
}
finally
{
fos.close();
}

The resulting code acts exactly the same as the original. If the
constructor for the FileOutputStream fails, then no cleanup will be
performed; otherwise, it will. However, in this version there are no
false initializations and no conditional in the finally block. (Also, I
couldn't resist the temptation to reformat to a more readable brace
style.)

--
www.designacourse.com
The Easiest Way To Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation

 

[Mike Schilling]

(Also, I
couldn't resist the temptation to reformat to a more readable brace
style.)

Here I disagree. Whatever brace style you prefer for "real" coding, for
Usenet the one that takes up fewer lines is superior.

 

[Chris Uppal]

Here I disagree. Whatever brace style you prefer for "real" coding, for
Usenet the one that takes up fewer lines is superior.

I wouldn't go along with that. The point -- whether on Usenet or in "real"
code -- is to be as readable as possible, not to be as short as possible.

I'll go so far as to agree that vertical whitespace can be minimised (compared
to code intended for reading "in bulk"), but that's about as far as I will go.
E.g. on Usenet I don't see anything wrong with trivial methods coded

int trivial() { return trivial; }

although I'd hate to see that in real code. But the K&R layout has not better
excuse on Usenet than it has anywhere else. To my mind it has no excuses at
all, but the point is that unless you (or Chris) genuinely believe it to the
most readable layout then you have no business choosing to use it in either
context. The same, of course, goes for any other layout.

-- chris

 

[Mark 'Kamikaze' Hughes]

I strongly disagree that it's a good idea to put a try/catch inside the
finally.
After all, what do you intend to do with the exception when you catch
it? Are you saying that the code that reads the file ought to have
knowledge of the exception handling strategy for the software?

Yes. You should handle exceptions as close to their cause as you
reasonably can.

Chris's Third Law of Exception Handling:

Knock this "law" shit off, it's juvenile. It's "Some Guy Named
Chris's Weird-Ass Personal Tastes in Exception Handling".

Trying to argue from authority when you're not an authority just makes
you look foolish.

FileOuputStream fos = new FileOutputStream(FILENAME);
try
{
... // write to file
}
finally
{
fos.close();
}
The resulting code acts exactly the same as the original. If the
constructor for the FileOutputStream fails, then no cleanup will be
performed; otherwise, it will. However, in this version there are no
false initializations and no conditional in the finally block. (Also, I
couldn't resist the temptation to reformat to a more readable brace
style.)

You are wrong. <http://java.sun.com/docs/codeconv/>

Look, I used to like the lined-up brace style, too, and in C people
are used to varying indentation styles, but it's *NOT* the normal format
for Java. 90% of the software projects anyone works on use the Sun
coding conventions, and you will cause needless strife with your
co-workers if you violate them because you think it looks prettier some
other way.

The point of code conventions is to make it easier to communicate. Do
not make it harder to communicate.

The lined-up style is really all about one thing, anyway--making it
easier to see where blocks start and end, so you can leave out the curly
braces in one-line blocks. That leads to this:
if(test)
doSomething();
doSomethingElse(); // THIS WON'T DO WHAT YOU THINK!

If you *always* use curly braces, it is just as easy to tell where
blocks start and end, and the indentation will always match the semantic
nesting:
if(test) {
doSomething();
doSomethingElse();
}

 

[Chris Smith]

Knock this "law" shit off, it's juvenile. It's "Some Guy Named
Chris's Weird-Ass Personal Tastes in Exception Handling".

I was joking. Sorry you lack a sense of humor. It must be
debilitating.

You are wrong. <http://java.sun.com/docs/codeconv/>

Look, I used to like the lined-up brace style, too, and in C people
are used to varying indentation styles, but it's *NOT* the normal format
for Java. 90% of the software projects anyone works on use the Sun
coding conventions, and you will cause needless strife with your
co-workers if you violate them because you think it looks prettier some
other way.

I am aware that Sun's coding conventions disagree on this point. While
most people adopt Sun's naming conventions, though, the remainder of the
coding conventions are far less universal. There's a good reason for
that; you can't effectively change Sun's naming conventions because
you'd have to use them anyway when accessing the standard API. The same
is not true for brace placement.

Anyway, it doesn't matter all that much. Obviously you are taking this
way too seriously. I write to give advice about exception handling. If
you want to talk about exception handling, then go for it.

--
www.designacourse.com
The Easiest Way To Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation

 

[Chris Smith]

Yes. You should handle exceptions as close to their cause as you
reasonably can.

I don't see any good reason for that. If I plan to handle problems in
code by emailing a problem report to an administrator and then updating
a status code on a monitoring web site somewhere, then I don't think the
code to do that should be scattered about the entire application.

Even worse (as I explained later in my first reply), you are potentially
ignoring serious problems in your code and continuing as if the file
write operating succeeded. Sure you can set a flag in the catch block
and then put an if statement after it, but then you've sort of missed
the point of using exceptions, haven't you?

--
www.designacourse.com
The Easiest Way To Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation

 

[Mike Schilling]

I wouldn't go along with that. The point -- whether on Usenet or in
"real"
code -- is to be as readable as possible, not to be as short as possible.

I'll go so far as to agree that vertical whitespace can be minimised
(compared
to code intended for reading "in bulk"), but that's about as far as I will
go.
E.g. on Usenet I don't see anything wrong with trivial methods coded

int trivial() { return trivial; }

although I'd hate to see that in real code. But the K&R layout has not
better
excuse on Usenet than it has anywhere else.

Other than minimising vertical whitespace, which was my point, and which you
apparently agree with.

 

[Lasse Reichstein Nielsen]

I don't see any good reason for that.

There are really two ways to deal with exceptions: catch them early or
late. It's the in-between that is the problem.

It's a matter of encapsulation. An exception thrown at some point in
the program is unlikely to be meaningfull at a higher level of
abstraction. If I use a datastructure internally in one of my
objects, then the clients of my object are unlikely to even know the
exact implementation of that datastructure. They will be completely
unsuited for handling the exception, and if I declare my methods
as throwing it, I'm making my implementation details public.
That is why the code that catches the exception should also understand
it, and that is most likely to happen close to the point where it
is thrown.

The exception to this is unchecked exeptions. An unchecked exception
represents an exceptional condition that whoever threw it considered
unexpectable and/or unrecoverable by the calling code. Again, it should
be caught by someone who knows what to do with it ... which in this
case would be either a top-level exception handler (e.g. in the
event queue) or nobody at all. The top-level handler would not understand
the exception's meaning, but would know what to do with it, by being
completely generic.

If I plan to handle problems in code by emailing a problem report to
an administrator and then updating a status code on a monitoring web
site somewhere, then I don't think the code to do that should be
scattered about the entire application.

No, that's the job of a single top-level handler. It doesn't understand
the exception, it merely propagates it outside the application. It might
allow the application to continue running, or it might not.

Even worse (as I explained later in my first reply), you are potentially
ignoring serious problems in your code and continuing as if the file
write operating succeeded.

Catching an exception without handling it is always wrong, whether you
catch it near or far. When you leave the "catch" block, your program
should continue in a desired direction, even when that is the exit.

/L

 

[Chris Smith]

There are really two ways to deal with exceptions: catch them early or
late. It's the in-between that is the problem.

I'm not sure I agree. Exceptions should, of course, be caught in the
"right" place, by which I mean the place that best balances various
development concerns. Sometimes that place is closer to the middle (for
example, five methods up, but not the seventeen methods all the way to
the root of the call tree.)

It's a matter of encapsulation. An exception thrown at some point in
the program is unlikely to be meaningfull at a higher level of
abstraction.

In that case, you catch the exception, wrap it, and rethrow. That
doesn't really count as catching for the purposes of this conversation.

No, that's the job of a single top-level handler. It doesn't understand
the exception, it merely propagates it outside the application. It might
allow the application to continue running, or it might not.

So, back in context again, the question is what to do with an exception
that is thrown during an OutputStream.close() operation. The options
were:

1. Don't catch it, and let it be handled elsewhere, realizing that this
could mask another I/O exception that was thrown earlier.

2. Catch it and continue without reacting (perhaps some logging or other
reporting could be done, but it would have to be done from a catch
clause inside a finally block at the leaves of the call tree).

3. Set a boolean flag of some sort, and write your own logic to replace
the normal throw/catch exception control flow.

I'm voting for #1. It leads to simpler code, less likelihood of missing
an important exception, and no substantial decrease in the likelihood of
having enough information to troubleshoot the original problem. It
doesn't really help for you to say that this should be done in a "top-
level handler", since the question is how to deal with issues in a
try/finally block, and we're not even dealing with how the exception is
handled from elsewhere in the call tree.

--
www.designacourse.com
The Easiest Way To Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation

 

[Mark 'Kamikaze' Hughes]

I was joking. Sorry you lack a sense of humor. It must be
debilitating.

I would suggest before you try what you call "joking" again, you learn
how to be funny. You were flaming with the light of a billion burning
nerds. Now that you've been called on it, you're just trying to
backpedal. That's "funny", in a sense, but only in the "we're all
laughing at you", not "we're all laughing with you" sense.

I am aware that Sun's coding conventions disagree on this point. While
most people adopt Sun's naming conventions, though, the remainder of the
coding conventions are far less universal. There's a good reason for
that; you can't effectively change Sun's naming conventions because
you'd have to use them anyway when accessing the standard API. The same
is not true for brace placement.

When you're working with someone else and using a non-standard
indentation style, they're going to have to hit reformat in Eclipse, and
leave a giant chunk of diff in the CVS logs. This is your fault for
violating standards.

Nobody in the entire world cares about your personal artistic
expression through indentation style, they just want a common,
consistent style so they can get their work done.

Anyway, it doesn't matter all that much. Obviously you are taking this
way too seriously. I write to give advice about exception handling. If
you want to talk about exception handling, then go for it.

I did. I also commented on your bizarre off-topic behaviors.