Is this an abuse of try/finally?
Given that multiple return statements are acceptable (I sort of disagree, but let us digress), I'm looking for a more acceptable way to achieve the following behavior:
Option A: multiple returns, repeated code block
public bool myMethod() {
/* ... code ... */
if(thisCondition) {
/* ... code that must run at end of method ... */
return false;
}
/* ... more code ... 开发者_JAVA技巧*/
if(thatCondition) {
/* ... the SAME code that must run at end of method ... */
return false;
}
/* ... even more code ... */
/* ... the SAME CODE AGAIN that must run at end of method ... */
return lastCondition;
}
It makes me feel dirty to see the same (little) code block repeated three times each time the method returns. Furthermore, I would like to clarify that the two return false
statements above can certainly be described as returning mid-method... they are absolutely not "guard statements."
Is Option B slightly more acceptable? I feel that I may abusing try/finally, and I'm hoping there is something completely different that I should be doing.
Option B: multiple returns, try/finally block (without catch blocks / exceptions)
public bool myMethod() {
try {
/* ... code ... */
if(thisCondition) {
return false;
}
/* ... more code ... */
if(thatCondition) {
return false;
}
/* ... even more code ... */
return lastCondition;
} finally {
/* ... code that must run at end of method ... */
}
}
Finally, Option C is the best solution in my book, but my team doesn't like this approach for whatever reason(s), hence I'm looking for a compromise.
Option C: single return, conditional blocks
public bool myMethod() {
/* ... code ... */
if(!thisCondition) {
/* ... more code ... */
}
if(!thisCondition && !thatCondition) {
/* ... even more code ... */
}
/* ... code that must run at end of method ... */
return summaryCondition;
}
If you want to discuss multiple return statements, please do so in this question.
If the code needs to run even when any other code throws an exception, then the finally
block is the correct solution.
If it need not run in the case of an exception (i.e. it's only necessary for "normal" returns), then using finally
would be abusing the feature.
Personally I'd rewrite that method in the single-return-point style. Not because I religiously subscribe to that idea (I don't), but because it is best suited for these kind of end-of-method codes.
When that code turns out to be overly complicated (and that's a very real possibility), then it's time to refactor the method by extracting one or more methods.
The simplest refactoring would be something like this:
public boolean myMethod() {
boolean result = myExtractedMethod();
/* ... code that must run at end of method ... */
return result;
}
protected boolean myExtractedMethod() {
/* ... code ... */
if(thisCondition) {
return false;
}
/* ... more code ... */
if(thatCondition) {
return false;
}
/* ... even more code ... */
return lastCondition;
}
Exceptions should be exceptional so I don't like option B if there are no other exceptions around(Note for downvoters - I don't say that having finally is incorrect just that I prefer not to have it if there are no exceptions - if you have reasons please comment)
If code is always needed how about refactoring into 2 functions
public bool myMethod() {
bool summaryCondition = myMethodWork();
// do common code
return summaryCondition;
}
private bool myMethodWork() {
/* ... code ... */
if(thisCondition) {
return false;
}
/* ... more code ... */
if(thatCondition) {
return false;
}
/* ... even more code ... */
return lastCondition;
}
This is a perfect place for a GOTO
*ducks*
Your option C solution is not far from optimal, since it adequately codes the proper execution sequence you're trying to accomplish.
Similarly, using nested if-statements do the same thing. It may be visually less appealing, but it's simpler to understand, and makes the execution flow pretty obvious:
public bool myMethod() {
boolean rc = lastCondition;
/* ... code-1 ... */
if (thisCondition) {
rc = false;
}
else {
/* ... code-2 ... */
if (thatCondition) {
rc = false;
}
else {
/* ... code-3 ... */
rc = ???;
}
}
/* ... the code that must run at end of method ... */
return rc;
}
Simplifying the code yields:
public bool myMethod() {
boolean rc = false;
/* ... code-1 ... */
if (!thisCondition) {
/* ... code-2 ... */
if (!thatCondition) {
/* ... code-3 ... */
rc = lastCondition;
}
}
/* ... the code that must run at end of method ... */
return rc;
}
The simplified code also reveals what you're actually trying to achieve: you are using the test conditions to avoid executing code, therefore you should probably be executing that code when the conditions are false instead of doing something when they are true.
To answer your question about try-finally blocks: Yes, you can abuse them. Your example is not sufficiently complex enough to warrant the use of try-finally. If it were more complex, though, it might.
See my take on it at: Go To Statement Considered Harmful: A Retrospective, "Exception Handling".
If the code needs to run even when there is an Exception
, then finally
is not just a good choice, it is a must. If that is not the case, finally
is not necessary. Looks like you want to find format that "looks" best. But there is little more at stake here.
How about breaking it up a little more to give something more ( excusing my not having used Java's logical operators in quite some time ) like this:
public bool findFirstCondition()
{
// do some stuff giving the return value of the original "thisCondition".
}
public bool findSecondCondition()
{
// do some stuff giving the return value of the original "thatCondition".
}
public bool findLastCondition()
{
// do some stuff giving the return value of the original "lastCondition".
}
private void cleanUp()
{
// perform common cleanup tasks.
}
public bool myMethod()
{
bool returnval = true;
returnval = returnval && findFirstCondition();
returnval = returnval && findSecondCondition();
returnval = returnval && findLastCondition();
cleanUp();
return returnval;
}
Don't abuse try/finally unless you need to break out of inner loops. Abuse do/while.
bool result = false;
do {
// Code
if (condition1) break;
// Code
if (condition2) break;
// . . .
result = lastCondition
} while (false);
Using try/finally for controlling flow feels to me like using GOTO.
Is there a reason that you can't simply store the return value, and fall out of the if?
bool retVal = true;
if (retVal && thisCondition) {
}
/* more code */
if ( retVal ) {
/* ... code that must run at end of method, maybe inside an if or maybe not... */
}
return retVal;
IMO the idea is to put a try
block over a small part of code (e.g. a method call) that can throw a known exception (e.g. reading from a file, reading an int
to a String
). So putting a try
block over the whole code of a method is really not the way to go, unless you are actually expecting each and every if
condition code to potentially throw the same set of exceptions. I don't see a point in using a try
block just for the sake of using a finally
.
If I am not mistaken, putting large chunks of code inside a try
also makes it a lot slower, but not sure if this is true in the latest versions of Java.
Personally, I would go with Option C. But I don't have anything against Option A either.
Unless the code that must run at the end of the method uses method local variables, you could extract it into a method like:
public boolean myMethod() {
/* ... code ... */
if(thisCondition) {
return myMethodCleanup(false);
}
/* ... more code ... */
if(thatCondition) {
return myMethodCleanup(false);
}
/* ... even more code ... */
return myMethodCleanup(lastCondition);
}
private boolean myMethodCleanup(boolean result) {
/* ... the CODE that must run at end of method ... */
return result;
}
this still doesn't look very nice, but it is better than using goto-like constructs. To convince your team mates that a 1-return solution might not be that bad, you can also present a version using 2 do { ... } while (false);
and break
's (*evil grin*.)
精彩评论