Java: Don't fold too complex expressions when extracting method (IDEA-175221, IDEA-167255)

java/java-analysis-impl/src/com/intellij/refactoring/extractMethod/ParametersFolder.java

@@ -225,22 +225,27 @@ public class ParametersFolder {
           if (isAccessedForWriting((PsiExpression)expression)) {
             return null;
           }
-          for (PsiElement scopeElement : scopeElements) {
-            if (PsiTreeUtil.isAncestor(expression, scopeElement, true)) {
-              expression = null;
-              break;
-            }
+          if (isAncestor(expression, scopeElements)) {
+            break;
+          }
+          if (dependsOnLocals(expression, inputVariables)) {
+            break;
+          }
+          final PsiElement parent = expression.getParent();
+          if (parent instanceof PsiExpressionStatement) {
+            break;
           }
-          if (expression == null) break;
-
           final PsiType expressionType = ((PsiExpression)expression).getType();
-          if (expressionType != null && !PsiType.VOID.equals(expressionType) && !(expression.getParent() instanceof PsiExpressionStatement)) {
-            if (dependsOnLocals(expression, inputVariables)) {
-              break;
-            }
+          if (expressionType == null || PsiType.VOID.equals(expressionType)) {
+            break;
+          }
+          if (isTooLongExpressionChain(expression)) {
+            break;
+          }
+          if (!isMethodNameExpression(expression)) {
             expressions.add((PsiExpression)expression);
           }
-          expression = PsiTreeUtil.getParentOfType(expression, PsiExpression.class);
+          expression = parent instanceof PsiExpression ? parent : null;
         }
       }
       else {
@@ -275,6 +280,47 @@ public class ParametersFolder {
     return exprWithWriteAccessInside[0] != null;
   }
 
+  private static boolean isAncestor(PsiElement expression, PsiElement[] scopeElements) {
+    for (PsiElement scopeElement : scopeElements) {
+      if (PsiTreeUtil.isAncestor(expression, scopeElement, true)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  private static boolean isTooLongExpressionChain(PsiElement expression) {
+    int count = 0;
+    for (PsiElement element = getInnerExpression(expression); element != null; element = getInnerExpression(element)) {
+      count++;
+      if (count > 1) { // expression chains like 'var.foo().bar()' and 'var.foo[i].bar()' are too long
+        return true;
+      }
+    }
+    return false;
+  }
+
+  private static PsiElement getInnerExpression(PsiElement expression) {
+    if (expression instanceof PsiMethodCallExpression) {
+      return ((PsiMethodCallExpression)expression).getMethodExpression().getQualifierExpression();
+    }
+    if (expression instanceof PsiArrayAccessExpression) {
+      while (expression instanceof PsiArrayAccessExpression) {
+        expression = ((PsiArrayAccessExpression)expression).getArrayExpression();
+      }
+      return expression;
+    }
+    return null;
+  }
+
+  private static boolean isMethodNameExpression(@NotNull PsiElement expression) {
+    final PsiElement parent = expression.getParent();
+    return expression instanceof PsiReferenceExpression &&
+           parent instanceof PsiMethodCallExpression &&
+           ((PsiReferenceExpression)expression).getReferenceNameElement() ==
+           ((PsiMethodCallExpression)parent).getMethodExpression().getReferenceNameElement();
+  }
+
   private static boolean dependsOnLocals(final PsiElement expression, final List<? extends PsiVariable> inputVariables) {
     final boolean[] localVarsUsed = new boolean[]{false};
     expression.accept(new JavaRecursiveElementWalkingVisitor(){

java/java-tests/testData/refactoring/extractMethod/BuilderChainWith2DimArrayAccess.java

@@ -0,0 +1,11 @@
+class Foo {
+    boolean bar(String[][] a) {
+        for (int i = 0; i < a.length; i++)
+            for (int j = 0; i < a[i].length; j++) {<selection>
+                if (a[i][j].length() > 3 && i % 3 == 0)
+                    return true;
+            </selection>
+        }
+        return false;
+    }
+}

java/java-tests/testData/refactoring/extractMethod/BuilderChainWith2DimArrayAccess_after.java

@@ -0,0 +1,16 @@
+class Foo {
+    boolean bar(String[][] a) {
+        for (int i = 0; i < a.length; i++)
+            for (int j = 0; i < a[i].length; j++) {
+                if (newMethod(a[i][j], i)) return true;
+
+            }
+        return false;
+    }
+
+    private boolean newMethod(String s, int i) {
+        if (s.length() > 3 && i % 3 == 0)
+            return true;
+        return false;
+    }
+}

java/java-tests/testData/refactoring/extractMethod/BuilderChainWithArrayAccess.java

@@ -0,0 +1,15 @@
+class UseBuilder {
+    void test(Builder builder, int[] arr) {
+        <selection>builder.foo("xyz").bar(arr[0]).foo("abc");</selection>
+    }
+
+    static class Builder {
+        Builder foo(String s) {
+            return this;
+        }
+
+        Builder bar(int x) {
+            return this;
+        }
+    }
+}

java/java-tests/testData/refactoring/extractMethod/BuilderChainWithArrayAccessExpr.java

@@ -0,0 +1,15 @@
+class UseBuilder {
+    void test(Builder builder, int[] arr) {
+        <selection>builder.foo("xyz").bar(arr[0])</selection>.foo("abc");
+    }
+
+    static class Builder {
+        Builder foo(String s) {
+            return this;
+        }
+
+        Builder bar(int x) {
+            return this;
+        }
+    }
+}

java/java-tests/testData/refactoring/extractMethod/BuilderChainWithArrayAccessExpr_after.java

@@ -0,0 +1,19 @@
+class UseBuilder {
+    void test(Builder builder, int[] arr) {
+        newMethod(builder, arr[0]).foo("abc");
+    }
+
+    private Builder newMethod(Builder builder, int x) {
+        return builder.foo("xyz").bar(x);
+    }
+
+    static class Builder {
+        Builder foo(String s) {
+            return this;
+        }
+
+        Builder bar(int x) {
+            return this;
+        }
+    }
+}

java/java-tests/testData/refactoring/extractMethod/BuilderChainWithArrayAccessIf.java

@@ -0,0 +1,9 @@
+class Foo {
+    boolean bar(String[] a) {
+        for (int i = 0; i < a.length; i++) {<selection>
+            if (a[i].length() > 3 && i % 3 == 0)
+                return true;
+        </selection>}
+        return false;
+    }
+}

java/java-tests/testData/refactoring/extractMethod/BuilderChainWithArrayAccessIf_after.java

@@ -0,0 +1,14 @@
+class Foo {
+    boolean bar(String[] a) {
+        for (int i = 0; i < a.length; i++) {
+            if (newMethod(a[i], i)) return true;
+        }
+        return false;
+    }
+
+    private boolean newMethod(String s, int i) {
+        if (s.length() > 3 && i % 3 == 0)
+            return true;
+        return false;
+    }
+}

java/java-tests/testData/refactoring/extractMethod/BuilderChainWithArrayAccess_after.java

@@ -0,0 +1,19 @@
+class UseBuilder {
+    void test(Builder builder, int[] arr) {
+        newMethod(builder, arr[0]);
+    }
+
+    private void newMethod(Builder builder, int x) {
+        builder.foo("xyz").bar(x).foo("abc");
+    }
+
+    static class Builder {
+        Builder foo(String s) {
+            return this;
+        }
+
+        Builder bar(int x) {
+            return this;
+        }
+    }
+}

java/java-tests/testData/refactoring/extractMethod/CallOn2DimArrayElement.java

@@ -0,0 +1,5 @@
+class C {
+    int foo(String[][] vars, int i, int j) {
+        return <selection>vars[i][j].length()</selection>;
+    }
+}

java/java-tests/testData/refactoring/extractMethod/CallOn2DimArrayElement_after.java

@@ -0,0 +1,9 @@
+class C {
+    int foo(String[][] vars, int i, int j) {
+        return newMethod(vars[i][j]);
+    }
+
+    private int newMethod(String s) {
+        return s.length();
+    }
+}

java/java-tests/testData/refactoring/extractMethod/CallOnArrayElement.java

@@ -0,0 +1,5 @@
+class C {
+    int foo(String[] vars, int i) {
+        return <selection>vars[i].length()</selection>;
+    }
+}

java/java-tests/testData/refactoring/extractMethod/CallOnArrayElement_after.java

@@ -0,0 +1,9 @@
+class C {
+    int foo(String[] vars, int i) {
+        return newMethod(vars[i]);
+    }
+
+    private int newMethod(String var) {
+        return var.length();
+    }
+}

java/java-tests/testData/refactoring/extractMethod/CallOnFieldArrayElement.java

@@ -0,0 +1,6 @@
+class C {
+    String[] vars;
+    int foo(C c, int i) {
+        return <selection>c.vars[i].length()</selection>;
+    }
+}

java/java-tests/testData/refactoring/extractMethod/CallOnFieldArrayElement_after.java

@@ -0,0 +1,10 @@
+class C {
+    String[] vars;
+    int foo(C c, int i) {
+        return newMethod(c.vars[i]);
+    }
+
+    private int newMethod(String var) {
+        return var.length();
+    }
+}

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

@@ -974,6 +974,34 @@ public class ExtractMethodTest extends LightCodeInsightTestCase {
     doTest();
   }
 
+  public void testBuilderChainWithArrayAccess() throws Exception {
+    doTest();
+  }
+
+  public void testBuilderChainWithArrayAccessExpr() throws Exception {
+    doTest();
+  }
+
+  public void testBuilderChainWithArrayAccessIf() throws Exception {
+    doTest();
+  }
+
+  public void testBuilderChainWith2DimArrayAccess() throws Exception {
+    doTest();
+  }
+
+  public void testCallOnArrayElement() throws Exception {
+    doTest();
+  }
+
+  public void testCallOn2DimArrayElement() throws Exception {
+    doTest();
+  }
+
+  public void testCallOnFieldArrayElement() throws Exception {
+    doTest();
+  }
+
   private void doTestDisabledParam() throws PrepareFailedException {
     final CodeStyleSettings settings = CodeStyleSettingsManager.getSettings(getProject());
     settings.ELSE_ON_NEW_LINE = true;