Community
Participate
Working Groups
Extract Local Variable puts the declaration at a wrong position iff - the selected expression is in an ExpressionStatement, and - there's a matching fragment before the selected expression public class ExpressionStatement { void m() { System.out.println(calculateCount()); calculateCount(); //Extract Local Variable } private int calculateCount() { return 1; } } The declaration is created at the position of the ExpressionStatement instead of before the first matching fragment. The fix is very local and the code to determine the right position is already there and can be reused.
Has to wait post 3.0
*** Bug 341979 has been marked as a duplicate of this bug. ***
(In reply to comment #0) > The fix is very local and the code to > determine the right position is already there and can be reused. Ping! It has been seven years :)
Created attachment 245185 [details] Proposed Patch The patch solves the sample case given in the initial description as follows (see also the included test cases): void m() { int temp= calculateCount(); System.out.println(temp); calculateCount(); //Extract Local Variable } In my opinion this behavior is preferable over: 1. Replacing the current expression with the temp variable: This will lead to a syntax error in all cases and therefore is not acceptable. 2. Deleting the current expression and placing the caret to the VariableDeclarationStatement: This will scroll up the source code in some cases which can lead to confusion and user frustration. I am also willed to improve the patch after a review and/or change the implementation if somebody provides some more clarification on the expected behavior.
There's also 'Quick Assist - Extract local variable (replace all occurrences)' action which has the same problem. I haven't tried the patch itself yet, but the end result looks confusing to me. I think option 2 should be re-evaluated keeping in mind that - there already exists the action 'Quick Assist - Extract local variable' which does the right thing with the test-case from comment 0 - the afore-mentioned 'replace all occurrences' version inserts the variable declaration line at the top and moves the caret there already, if the test-case is modified to this: void m() { System.out.println(calculateCount()); int i = calculateCount(); //Extract Local Variable } One other option could also be to override the 'replace all occurrences' facet of the refactoring in this case, producing this end result from the test-case from comment 0: void m() { System.out.println(calculateCount()); int temp = calculateCount(); //Extract Local Variable } I think I'd prefer option 2 but I'll have to play around with the patch to know for sure.
Thanks for the patch, Nico! Released as http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=8d894e351091ffa0f74b573db4b6d96be7f0a1e3 I agree it's probably better to leave ExpressionStatements (and the other cases in ExtractTempRefactoring#canReplace(..)) alone, rather than taking option 2 and removing them. Both solutions can leave the user puzzled, but the current solution at least doesn't remove code that may be necessary. Anyway, this would have to be done in a separate bug, and it would have to work in special cases like this as well: void m(boolean a) { int x= calculateCount(); if (a) calculateCount(); // not replaced else return; }
