Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 366281

Summary:	[refactoring] [extract method] Extract Method refactoring fails if trailing ';' is not selected
Product:	[Eclipse Project] JDT	Reporter:	Mohsen Vakilian <reprogrammer>
Component:	UI	Assignee:	Deepak Azad <deepakazad>
Status:	RESOLVED FIXED	QA Contact:
Severity:	normal
Priority:	P3	CC:	deepakazad, djanoiup, nchen, reprogrammer, snegara2
Version:	3.8
Target Milestone:	3.8 M5
Hardware:	PC
OS:	All
Whiteboard:

Description Mohsen Vakilian

2011-12-09 22:37:23 EST

Build ID: I20111207-2118

If a programmer selects the left-hand side "i" from a statement of form "i = 0;" and invokes the Extract Method refactoring, the refactoring tool will report the following problem and prevent the programmer from continuing the refactoring:

>Cannot extract the left-hand side of an assignment.

However, we noticed that the refactoring tool will report the above error message even if the programmer selects an expression of form "i = 0" from a statement of form "i = 0;". Two of the participants of the CodingSpectator study <http://codingspectator.cs.illinois.edu/> forgot to include the trailing semicolon in their selections and ran into this problem. This problem occurred three times. Because our participants sometimes repeated the refactoring multiple times, they received the above error message five times. In all three instances, our participants eventually overcome the problem by including the trailing semicolon in their selections.

It would be nice if the refactoring automatically extends the selection to cover the trailing semicolon the same way that it shrinks the selection to exclude the trailing semicolon in some cases (See Bug 324237).

Comment 1 Deepak Azad

2011-12-16 06:36:34 EST

The if condition in ExtractMethodAnalyzer.visit(Assignment) does not look perfect.

Comment 2 Jean-Noel Rouvignac

2012-01-20 03:03:14 EST

What about using this:

if ((getSelection().covers(node.getLeftHandSide()) && !getSelection().covers(node.getRightHandSide())) || getSelection().coveredBy(node.getLeftHandSide())) {

Comment 3 Deepak Azad

2012-01-21 10:15:16 EST

(In reply to comment #2)
> if ((getSelection().covers(node.getLeftHandSide()) &&
> !getSelection().covers(node.getRightHandSide())) ||
> getSelection().coveredBy(node.getLeftHandSide())) {

Thanks! That works.

Fixed in master - 371ad683cf46d904a9c00c3494d31522d33f5b08

Note that the result is slightly different when you select 'i = 0;' or 'i = 0', but I think that is OK.

Comment 4 Mohsen Vakilian

2012-01-21 10:31:03 EST

(In reply to comment #3)
> Note that the result is slightly different when you select 'i = 0;' or 'i = 0'

How are the results different?

Comment 5 Deepak Azad

2012-01-21 10:44:41 EST

(In reply to comment #4)
> How are the results different?

New method with ; selected
private void extracted() {
	field = 1;
}

New method with ; NOT selected
private int extracted() {
	return field = 1;
}

Full code snippets are available here - http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=371ad683cf46d904a9c00c3494d31522d33f5b08

Comment 6 Mohsen Vakilian

2012-01-21 16:03:16 EST

(In reply to comment #5)
> (In reply to comment #4)
> > How are the results different?
> 
> New method with ; selected
> private void extracted() {
>     field = 1;
> }
> 
> New method with ; NOT selected
> private int extracted() {
>     return field = 1;
> }
> 
> Full code snippets are available here -
> http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=371ad683cf46d904a9c00c3494d31522d33f5b08

Deepak,

Thanks for pointing me to the examples. The different behaviors in the above two cases seem consistent with how Eclipse handles other similar selections. I will try a few more examples when a build of Eclipse that contains your patch appears at http://download.eclipse.org/eclipse/downloads/