extract method object: conditional exit; nullable check should be processed separately ( IDEA-52452 )

java/java-impl/src/com/intellij/refactoring/extractMethod/ExtractMethodProcessor.java

@@ -116,7 +116,7 @@ public class ExtractMethodProcessor implements MatchProvider {
   private DuplicatesFinder myDuplicatesFinder;
   private List<Match> myDuplicates;
   @Modifier private String myMethodVisibility = PsiModifier.PRIVATE;
-  private boolean myGenerateConditionalExit;
+  protected boolean myGenerateConditionalExit;
   private PsiStatement myFirstExitStatementCopy;
   private PsiMethod myExtractedMethod;
   private PsiMethodCallExpression myMethodCall;
@@ -633,6 +633,7 @@ public class ExtractMethodProcessor implements MatchProvider {
           myMethodCall =
             (PsiMethodCallExpression)((PsiAssignmentExpression)assignmentExpression.getExpression()).getRExpression().replace(myMethodCall);
         }
+        declareNecessaryVariablesAfterCall(myOutputVariable);
         PsiIfStatement ifStatement =
           (PsiIfStatement)myElementFactory.createStatementFromText(myHasReturnStatementOutput || (myGenerateConditionalExit && myFirstExitStatementCopy instanceof PsiReturnStatement &&
                                                                                                   ((PsiReturnStatement)myFirstExitStatementCopy).getReturnValue() != null)
@@ -685,7 +686,9 @@ public class ExtractMethodProcessor implements MatchProvider {
         addToMethodCallLocation(exitStatementCopy);
       }
 
-      declareNecessaryVariablesAfterCall(myOutputVariable);
+      if (!myNullConditionalCheck) {
+        declareNecessaryVariablesAfterCall(myOutputVariable);
+      }
 
       deleteExtracted();
     }

java/java-impl/src/com/intellij/refactoring/extractMethodObject/ExtractMethodObjectProcessor.java

@@ -21,6 +21,7 @@
 package com.intellij.refactoring.extractMethodObject;
 
 import com.intellij.codeInsight.AnnotationUtil;
+import com.intellij.openapi.application.ApplicationManager;
 import com.intellij.openapi.diagnostic.Logger;
 import com.intellij.openapi.editor.Editor;
 import com.intellij.openapi.project.Project;
@@ -41,14 +42,13 @@ import com.intellij.psi.util.PsiTreeUtil;
 import com.intellij.psi.util.PsiUtil;
 import com.intellij.refactoring.BaseRefactoringProcessor;
 import com.intellij.refactoring.HelpID;
-import com.intellij.refactoring.classMembers.MemberInfoBase;
 import com.intellij.refactoring.changeSignature.ChangeSignatureProcessor;
 import com.intellij.refactoring.changeSignature.ParameterInfoImpl;
+import com.intellij.refactoring.classMembers.MemberInfoBase;
 import com.intellij.refactoring.extractMethod.AbstractExtractDialog;
 import com.intellij.refactoring.extractMethod.ExtractMethodProcessor;
 import com.intellij.refactoring.ui.MemberSelectionPanel;
 import com.intellij.refactoring.util.RefactoringUtil;
-import com.intellij.util.VisibilityUtil;
 import com.intellij.refactoring.util.classMembers.MemberInfo;
 import com.intellij.refactoring.util.duplicates.Match;
 import com.intellij.usageView.UsageInfo;
@@ -57,6 +57,7 @@ import com.intellij.usageView.UsageViewUtil;
 import com.intellij.util.ArrayUtil;
 import com.intellij.util.Function;
 import com.intellij.util.IncorrectOperationException;
+import com.intellij.util.VisibilityUtil;
 import org.jetbrains.annotations.NonNls;
 import org.jetbrains.annotations.NotNull;
 
@@ -150,6 +151,11 @@ public class ExtractMethodObjectProcessor extends BaseRefactoringProcessor {
           }
         }
 
+        if (myExtractProcessor.generatesConditionalExit()) {
+          myInnerClass.add(myElementFactory.createField("myResult", PsiPrimitiveType.BOOLEAN));
+          myInnerClass.add(myElementFactory.createMethodFromText("boolean is(){return myResult;}", myInnerClass));
+        }
+
         final PsiParameter[] parameters = getMethod().getParameterList().getParameters();
         if (parameters.length > 0) {
           createInnerClassConstructor(parameters);
@@ -181,8 +187,17 @@ public class ExtractMethodObjectProcessor extends BaseRefactoringProcessor {
     }
   }
 
-  protected void moveUsedMethodsToInner() {
+  public void moveUsedMethodsToInner() {
     if (!myUsages.isEmpty()) {
+      if (ApplicationManager.getApplication().isUnitTestMode()) {
+        for (MethodToMoveUsageInfo usage : myUsages) {
+          final PsiMember member = (PsiMember)usage.getElement();
+          LOG.assertTrue(member != null);
+          myInnerClass.add(member.copy());
+          member.delete();
+        }
+        return;
+      }
       final List<MemberInfo> memberInfos = new ArrayList<MemberInfo>();
       for (MethodToMoveUsageInfo usage : myUsages) {
         memberInfos.add(new MemberInfo((PsiMethod)usage.getElement()));
@@ -244,11 +259,13 @@ public class ExtractMethodObjectProcessor extends BaseRefactoringProcessor {
     LOG.assertTrue(body != null);
     final List<PsiLocalVariable> vars = new ArrayList<PsiLocalVariable>();
     final Map<PsiElement, PsiElement> replacementMap = new LinkedHashMap<PsiElement, PsiElement>();
+    final List<PsiReturnStatement> returnStatements = new ArrayList<PsiReturnStatement>();
     body.accept(new JavaRecursiveElementWalkingVisitor() {
       @Override
       public void visitReturnStatement(final PsiReturnStatement statement) {
         super.visitReturnStatement(statement);
         try {
+          returnStatements.add(statement);
           replacementMap.put(statement, myElementFactory.createStatementFromText("return this;", statement));
         }
         catch (IncorrectOperationException e) {
@@ -294,6 +311,17 @@ public class ExtractMethodObjectProcessor extends BaseRefactoringProcessor {
       }
     });
 
+    if (myExtractProcessor.generatesConditionalExit()) {
+      for (int i = 0; i < returnStatements.size() - 1; i++) {
+        final PsiReturnStatement condition = returnStatements.get(i);
+        condition.getParent().addBefore(myElementFactory.createStatementFromText("myResult = true;", condition), condition);
+      }
+
+      LOG.assertTrue(!returnStatements.isEmpty());
+      final PsiReturnStatement returnStatement = returnStatements.get(returnStatements.size() - 1);
+      returnStatement.getParent().addBefore(myElementFactory.createStatementFromText("myResult = false;", returnStatement), returnStatement);
+    }
+
     for (PsiLocalVariable var : vars) {
       final String fieldName = var2FieldNames.get(var.getName());
       for (PsiReference reference : ReferencesSearch.search(var)) {
@@ -652,8 +680,15 @@ public class ExtractMethodObjectProcessor extends BaseRefactoringProcessor {
         final String object = StringUtil.decapitalize(myInnerClassName);
         final PsiStatement methodCallStatement = PsiTreeUtil.getParentOfType(getMethodCall(), PsiStatement.class);
         LOG.assertTrue(methodCallStatement != null);
-        methodCallStatement.replace(
-          myElementFactory.createStatementFromText(myInnerClassName + " " + object + " = " + getMethodCall().getText() + ";", myInnerMethod));
+        final PsiStatement declarationStatement = myElementFactory
+          .createStatementFromText(myInnerClassName + " " + object + " = " + getMethodCall().getText() + ";", myInnerMethod);
+        if (methodCallStatement instanceof PsiIfStatement) {
+          methodCallStatement.getParent().addBefore(declarationStatement, methodCallStatement);
+          final PsiExpression conditionExpression = ((PsiIfStatement)methodCallStatement).getCondition();
+          conditionExpression.replace(myElementFactory.createExpressionFromText(object + ".is()", myInnerMethod));
+        } else {
+          methodCallStatement.replace(declarationStatement);
+        }
 
         final List<PsiVariable> usedVariables = myControlFlowWrapper.getUsedVariables();
         Collection<ControlFlowUtil.VariableInfo> reassigned = myControlFlowWrapper.getInitializedTwice();
@@ -687,5 +722,8 @@ public class ExtractMethodObjectProcessor extends BaseRefactoringProcessor {
       }
     }
 
+    public boolean generatesConditionalExit() {
+      return myGenerateConditionalExit;
+    }
   }
 }

java/java-tests/testData/refactoring/extractMethodObject/WithPrivateMethodUsed.java

@@ -0,0 +1,9 @@
+public class XXX {
+  void f<caret>oo() {
+    int i = 0 ;
+    bar(i);
+    System.out.println(i);
+  }
+
+  private void bar(int i){}
+}

java/java-tests/testData/refactoring/extractMethodObject/WithPrivateMethodUsed.java.after

@@ -0,0 +1,15 @@
+public class XXX {
+  void foo() {
+      new InnerClass().invoke();
+  }
+
+    private class InnerClass {
+        public void invoke() {
+            int i = 0 ;
+            bar(i);
+            System.out.println(i);
+        }
+
+        private void bar(int i){}
+    }
+}

java/java-tests/testData/refactoring/extractMethodObject/WithPrivateMethodWhichCantBeMoved.java

@@ -0,0 +1,13 @@
+public class XXX {
+  void f<caret>oo() {
+    int i = 0 ;
+    bar(i);
+    System.out.println(i);
+  }
+
+  void bazz() {
+    bar(0);
+  }
+
+  private void bar(int i){}
+}

java/java-tests/testData/refactoring/extractMethodObject/WithPrivateMethodWhichCantBeMoved.java.after

@@ -0,0 +1,19 @@
+public class XXX {
+  void foo() {
+      new InnerClass().invoke();
+  }
+
+    void bazz() {
+    bar(0);
+  }
+
+  private void bar(int i){}
+
+    private class InnerClass {
+        public void invoke() {
+            int i = 0 ;
+            bar(i);
+            System.out.println(i);
+        }
+    }
+}

java/java-tests/testData/refactoring/extractMethodObject/multipleExitPoints/ConditionalExit.java

@@ -0,0 +1,9 @@
+class Foo {
+  String foo() {
+    <selection>String var = null;
+    if (var == null) {
+      return "";
+    }</selection>
+    System.out.println(var);
+  }
+}

java/java-tests/testData/refactoring/extractMethodObject/multipleExitPoints/ConditionalExit.java.after

@@ -0,0 +1,31 @@
+class Foo {
+  String foo() {
+      Inner inner = new Inner().invoke();
+      if (inner.is()) return "";
+      String var = inner.getVar();
+    System.out.println(var);
+  }
+
+    private class Inner {
+        private boolean myResult;
+        private String var;
+
+        boolean is() {
+            return myResult;
+        }
+
+        public String getVar() {
+            return var;
+        }
+
+        public Inner invoke() {
+            var = null;
+            if (var == null) {
+                myResult = true;
+                return this;
+            }
+            myResult = false;
+            return this;
+        }
+    }
+}

java/java-tests/testData/refactoring/extractMethodObject/multipleExitPoints/OutputVariable.java

@@ -0,0 +1,9 @@
+class Foo {
+  String foo() {
+    <selection>String var = "";
+    if (var == null) {
+      return null;
+    }</selection>
+    System.out.println(var);
+  }
+}

java/java-tests/testData/refactoring/extractMethodObject/multipleExitPoints/OutputVariable.java.after

@@ -0,0 +1,31 @@
+class Foo {
+  String foo() {
+      Inner inner = new Inner().invoke();
+      String var = inner.getVar();
+      if (var == null) return null;
+    System.out.println(var);
+  }
+
+    private class Inner {
+        private boolean myResult;
+        private String var;
+
+        boolean is() {
+            return myResult;
+        }
+
+        public String getVar() {
+            return var;
+        }
+
+        public Inner invoke() {
+            var = "";
+            if (var == null) {
+                myResult = true;
+                return this;
+            }
+            myResult = false;
+            return this;
+        }
+    }
+}

java/java-tests/testSrc/com/intellij/refactoring/ExtractMethodObjectTest.java

@@ -37,6 +37,9 @@ public class ExtractMethodObjectTest extends LightCodeInsightTestCase {
     extractProcessor.testRun();
     processor.setCreateInnerClass(createInnerClass);
     processor.run();
+    if (createInnerClass) {
+      processor.moveUsedMethodsToInner();
+    }
     DuplicatesImpl.processDuplicates(extractProcessor, getProject(), getEditor());
     processor.getMethod().delete();
     checkResultByFile("/refactoring/extractMethodObject/" + testName + ".java" + ".after");
@@ -101,4 +104,12 @@ public class ExtractMethodObjectTest extends LightCodeInsightTestCase {
   public void testWrapWithObjectRecursive() throws Exception {
     doTest(false);
   }
+  
+  public void testWithPrivateMethodUsed() throws Exception {
+    doTest();
+  }
+
+  public void testWithPrivateMethodWhichCantBeMoved() throws Exception {
+    doTest();
+  }
 }

java/java-tests/testSrc/com/intellij/refactoring/ExtractMethodObjectWithMultipleExitPointsTest.java

@@ -66,4 +66,11 @@ public class ExtractMethodObjectWithMultipleExitPointsTest extends LightCodeInsi
     doTest();
   }
 
+  public void testConditionalExit() throws Exception {
+    doTest();
+  }
+
+  public void testOutputVariable() throws Exception {
+    doTest();
+  }
 }

java/java-tests/testSrc/com/intellij/refactoring/ReplaceMethodDuplicatesTestSuite.java

@@ -16,6 +16,7 @@ public class ReplaceMethodDuplicatesTestSuite {
     testSuite.addTestSuite(ExtractMethod15Test.class);
     testSuite.addTestSuite(ExtractMethodTest.class);
     testSuite.addTestSuite(ExtractMethodObjectWithMultipleExitPointsTest.class);
+    testSuite.addTestSuite(ExtractMethodObjectTest.class);
     testSuite.addTestSuite(FindMethodDuplicatesMiscTest.class);
     testSuite.addTestSuite(FindMethodDuplicatesTest.class);
     return testSuite;