Bug 48056 - [extract method] Test cases for extract method and continue
Summary: [extract method] Test cases for extract method and continue
Status: ASSIGNED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.0   Edit
Hardware: All All
: P3 enhancement with 1 vote (vote)
Target Milestone: ---   Edit
Assignee: Benjamin Muskalla CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 54947 (view as bug list)
Depends on:
Blocks:
 
Reported: 2003-12-04 06:46 EST by David Corbin CLA
Modified: 2011-04-26 05:30 EDT (History)
3 users (show)

See Also:


Attachments
patch+testcases (37.54 KB, patch)
2009-07-25 16:03 EDT, Benjamin Muskalla CLA
markus.kell.r: iplog+
Details | Diff
Patch 2 (42.54 KB, patch)
2009-08-03 05:16 EDT, Markus Keller CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Corbin CLA 2003-12-04 06:46:07 EST
I have a block of code like this:

for (...)
{
    some 
    code
    /*[*/
    if (condition)
        continue;
    more 
    code
    here
    /*]*/
}

and I do an extract method on the bracket surrounded code (I hope I remembered
the notation correctly).  It fails with an error message that essentially says
it can't do this because of the continue.  But, beceause the selection runs to
the end of the for block, it should be able to handle this by translating the
continue to a return (provided the new method returns void).
Comment 1 Dirk Baeumer CLA 2004-03-16 13:01:10 EST
*** Bug 54947 has been marked as a duplicate of this bug. ***
Comment 2 Markus Keller CLA 2004-11-10 10:39:04 EST
This would really be useful when refactoring spaghetti-code.
Comment 3 Dirk Baeumer CLA 2004-11-11 05:11:31 EST
Agree. Will see if we can do something for 3.1
Comment 4 Benjamin Muskalla CLA 2009-07-25 16:03:16 EDT
Created attachment 142588 [details]
patch+testcases

Ok, finally I think I got all edge cases covered ;-)
Markus, do you still see any case I missed?
Any thoughts on the patch?
Comment 5 Markus Keller CLA 2009-08-03 05:16:47 EDT
Created attachment 143249 [details]
Patch 2

Thanks, I've released the patch with a few bug fixes and a few stylistic nitpicks:
- ExtractMethodAnalyzer#canHandleBranches():
  - early returns are easier to understand
  - "if (body != null) { if (body instanceof Block)...": null check is unnecessary
- updated the error messages

Example with remaining problems I fixed:

public static void main(String[] args) {
	outer: for (int i= 0; i < 3; i++) {
		/*[*/
		inner: for (int j= 0; j < 3; j++) {
//			while (i == j) {} //made both continues become returns
			if (i == 1)
				continue outer;
			if (j == 2)
				continue inner;
			System.out.println(i + ", " + j);
//			break outer; //must block!
//			break inner; //must block!
			break;
		}
		/*]*/
	}
}

- Patch did not allow me to extract the whole block (including curly braces) of the outer loop: { /*[*/ ... /*]*/ }
- If you switched the order of the two continue statements, the refactoring still showed the error message, although extracting works fine.
- Did not handle break statements (we don't support breaking out of outer loops)
  - I did not verify breaks inside switch statements. Can you please check whether we have a test for that?
- The ASTVisitor in ExtractMethodRefactoring#replaceBranches(CompilationUnitChange) failed when
  - continue statement was after a nested loop
  - labelled loop was not a for loop, but enhaced for, do, or while

If you could attach another patch with test cases for these, that would be great!
Comment 6 Dani Megert CLA 2010-02-02 06:19:51 EST
Benny, any plans to provide tests?
Comment 7 Dani Megert CLA 2011-04-12 06:29:41 EDT
Ping! Benny, any plans to work on this?