Just to be the first one to say "thanks" on a feature...

I figured I'd let you guys know that the improvements to Extract Method simply rule. Ability to extract gaurd and tail sequences is going to make defensive coding much easier to manage. Thanks.

--Dave Griffith

16 comments

Thank you, and thank you for choosing this particular feature to be the first to mention:)

0

My thank you goes to the love-that-dares-not-speak-its-name that is now manifesting itself between IDEA and OSX. Hooray for the native looking progress bar, and an even bigger hooray for the lack of flashing once per module on startup that's plagued IDEA since 4.x.

Of course, I wouldn't be me if I didn't boo some things either, so I'd like to boo the javascript checking dialog that pops up when I add a JDK, and the astoundingly sluggish performance of Demetra on my pre-historic powerbook.

0

In article <29925562.1139259531959.JavaMail.itn@is.intellij.net>,
dave.griffith@cnn.com says...

I figured I'd let you guys know that the improvements to Extract Method simply rule. Ability to extract gaurd and tail sequences is going to make defensive coding much easier to manage. Thanks.

--Dave Griffith

Could you explain?
--
-


David H. McCoy


-


0

Sure.

Extract Method is the most powerful refactoring available(*), but it has always been more constrained than it needed to be. In particular, pre-Demetra implementations required that the statements extracted contain only one return point. In the general case, that's necessary, since Java has no way of abstracting out return locations. (A language with first-class continuations could abstract out multi-return statements into separate methods, but you would probably end up with something more complex and unmaintainable than what you started with. Ewww.)

There are two special common special cases, though, where multi-return statement sequences can be extracted cleanly. If the sequence always returns (or throws exceptions), then it can safely be extracted. This commonly happens at the end of a method, and thus I've been calling it tail-extraction, although under certain circumstances it can occur in the middle of a method, say as one branch of a complex if-statement. Another special case occurs when a sequence of statements may either return or not, and doesn't have any other visible effects. In that case, the sequence can be extracted to a boolean method, which returns true if the sequence returned and false if it didn't. This is a common pattern for testing gaurd conditions before performing some action, so I've been calling it gaurd-extraction.

For example, say I've got a method that looks like


InspectionGadgets basically consists of about 1000 different methods that all look more or less like that, and some scaffolding to hook them to your editor.
Before Demetra, there just wasn't anything that Extract Method could do to simplify that. Anything you tried to extract, you would be told "Multiple return points! Denied!" Post-Demetra, literally any of subsequences of statements in the above can be extracted, allowing you to clarify your logic greatly. My comment on defensive programming is basically that defensively coded methods tend to have a lot of little "if" statements, and this new functionality will allow those to be grouped, abstracted and merged more or less arbitrarily.

(*) Extract Class in Refactor-J comes close.

--Dave Griffith

0

note that also

public void foo()
{
if(bar()) return foo();
if(baz()) return foo();
if(barzoom()) return foo();
if(barangus()) return foo();
doSomething();
}

is equally legal to be extracted.

0

What about something like this where there is a mixture of types (all subtypes of the return type) plus null return value?

}

0

Similar example, except using interface List as return type with various concrete implementations being returned.
Extract Method won't work on lines 1-2, 2-3,1-3,3-4,1-4 fragments.

0

There is no way how method could be extracted in this situation

0

Oops. I guess I got carried away playing with your examples. I see why it can't be extracted (at least as a single method).

Shouldn't it also prevent me from Extract Method on lines 2-3,2-4,3-4,etc. of foo() in this slightly modified version?


If I extract lines 2-4 I get

which isn't right because result doesn't get set when the conditions in getList() are true. Extract Method can't return the result either because in the case the conditions are false, result shouldn't be changed in foo().

I think it works like this in 5.1 also.

0

Uh?
(I'm asking because your non-compiling example could be interpreted in a few
different ways)

Vince.


0


Here's complete example. In Demetra EAP, you can now extract out a subset of foo(), e.g. lines 1-2, lines 2-3, lines 1-3, etc. If you try that in 5.1, you will get the error message "Cannot perform refactoring. There are multiple exit points in the selected code fragment."

Eugene's point was that you could extract out statements with multiple return points not only in the case where they return nothing (Dave's example) but also if they return the same thing.

0

Eugene,

Here is a minor quibble


Example: Extract lines 2+3 of foo()
5.1 : Gives error message
6.0 : Extracts the method. By default it wants to include 'result' as a parameter even though it is unused by the extracted method.

Expected: result shouldn't be listed as a parameter, or at least it should be unselected by default.

This is what Demetra produces if I accept the default:


0

Here is an annoying quirk.
If you have a comment at the end of the line, Extract Method says "Cannot perform refactoring. There are multiple exit points in the selected code fragement."



Same problem if I use /* line1 */ comment style.

0

If I highlight two of the if statements below including the comment lines and invoke Extract Method, I get an Exception.

(DuplicatesFinder.java:36)
	at com.intellij.refactoring.extractMethod.ExtractMethodProcessor.prepare(ExtractMethodProcessor.java:174)
	at com.intellij.refactoring.extractMethod.ExtractMethodHandler.a(ExtractMethodHandler.java:48)
	at com.intellij.refactoring.extractMethod.ExtractMethodHandler.invoke(ExtractMethodHandler.java:18)
	at com.intellij.refactoring.actions.BaseRefactoringAction.actionPerformed(BaseRefactoringAction.java:44)
]]>



Attachment(s):
5131_exception_on_extract_method.png
0

Looking at this again, I realize this is a pathological case because the statement

makes the code above pointless (unless an exception occurred before the end of the method).
If I remove the else statement at the end, then Extract Method does the right thing.

Here is a more compact example of the above:


If YOu Extract lines 2+3 of foo(), it creates a local variable Object result in the extracted method which
is logically equivalent, although I think it might be technically incorrect because there could be a RunTime exeption that occurs before foo() ends, and then result might not have the same value as the unextracted version.


If I removed the else statement at the end of foo, and performed the extract method again, then it is correct:

0

Fixed.
Also added duplication extraction for the code like:

class Test
{
public Object foo() {
if(test1()) return null; if(test2()) return null;]]>
return new Object();
}

public int foo1() {
if(test1()) return 0;
if(test2()) return 0;
return 1;
}

private boolean test1() {return false;}
private boolean test2() {return false;}
}

0

Please sign in to leave a comment.