IDEA-274123 [extract method]: don't suggest too abstract change signatures

GitOrigin-RevId: 4c59710e2304288ad4465a92ec36bff633bd661e

java/java-impl-refactorings/src/com/intellij/refactoring/extractMethod/newImpl/inplace/DuplicatesMethodExtractor.kt

@@ -109,7 +109,9 @@ class DuplicatesMethodExtractor(val extractOptions: ExtractOptions, val anchor:
       return dialog.showAndGet()
     }
     val confirmChange: () -> Boolean = changeSignatureDefault?.let { default -> {default} } ?: ::confirmChangeSignature
-    val changeSignature = parametrizedDuplicatesNumber > 0 && confirmChange()
+    val isGoodSignatureChange = isGoodSignatureChange(extractOptions.elements, extractOptions.inputParameters,
+                                                      parametrizedExtraction.callElements, updatedParameters)
+    val changeSignature = parametrizedDuplicatesNumber > 0 && isGoodSignatureChange && confirmChange()
     duplicates = if (changeSignature) duplicatesWithUnifiedParameters else exactDuplicates
     val parameters = if (changeSignature) updatedParameters else extractOptions.inputParameters
     val extractedElements = if (changeSignature) parametrizedExtraction else ExtractedElements(calls, method)
@@ -140,6 +142,21 @@ class DuplicatesMethodExtractor(val extractOptions: ExtractOptions, val anchor:
     }
   }
 
+  private fun isGoodSignatureChange(callBefore: List<PsiElement>, initialParameters: List<InputParameter>,
+                                    callAfter: List<PsiElement>, updatedParameters: List<InputParameter>): Boolean {
+    val sizeAfter = callAfter.sumOf(::calculateCodeLeafs)
+    val sizeBefore = callBefore.sumOf(::calculateCodeLeafs)
+    val addedParameters = updatedParameters.size - initialParameters.size
+    return 1.75 * sizeAfter < sizeBefore && addedParameters <= 3 && updatedParameters.size <= 5
+  }
+
+  private fun calculateCodeLeafs(element: PsiElement): Int {
+    return SyntaxTraverser.psiTraverser(element)
+      .filter { psiElement -> psiElement.firstChild == null && psiElement.text.isNotBlank() }
+      .traverse()
+      .count()
+  }
+
   private fun findMethodCallInside(element: PsiElement?): PsiMethodCallExpression? {
     return PsiTreeUtil.findChildOfType(element, PsiMethodCallExpression::class.java, false)
   }

java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/DuplicatedButDeclined_after.java

@@ -1,12 +1,12 @@
 public class Test {
     void test(boolean condition) {
-        extracted("one");
+        extracted();
         foo("one");
         foo("two");
     }
 
-    private void extracted(String one) {
-        foo(one);
+    private void extracted() {
+        foo("one");
     }
 
     void foo(String name) {

java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/DuplicatedExpressionAndChangeSignature.java

@@ -1,6 +1,6 @@
 class Test {
-  public void test() {
-    byte[] one = <selection>"one".getBytes()</selection>;
-    byte[] two = "two".getBytes();
-  }
+    public void test() {
+        String start1 = <selection>"one".substring(0, 10)</selection>;
+        String start2 = "two".substring(0, 10);
+    }
 }

java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/DuplicatedExpressionAndChangeSignature_after.java

@@ -1,13 +1,13 @@
 import org.jetbrains.annotations.NotNull;
 
 class Test {
-  public void test() {
-    byte[] one = getBytes("one");
-    byte[] two = getBytes("two");
-  }
+    public void test() {
+        String start1 = getSubstring("one");
+        String start2 = getSubstring("two");
+    }
 
     @NotNull
-    private static byte[] getBytes(String one) {
-        return one.getBytes();
+    private static String getSubstring(String one) {
+        return one.substring(0, 10);
     }
 }

java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/RenamedParametrizedDuplicate.java

@@ -1,6 +1,8 @@
 class Test {
+    static int offset = 42;
+
     void test(){
-        int avgA = <selection>10 + 20 / 2</selection>;
-        int avgB = 100 + 200 / 2;
+        int avgA = <selection>10 + 20 / 2 - Test.offset - 1</selection>;
+        int avgB = 100 + 200 / 2 - Test.offset - 1;
     }
 }

java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/RenamedParametrizedDuplicate_after.java

@@ -1,10 +1,12 @@
 class Test {
+    static int offset = 42;
+
     void test(){
-        int avgA = average(10, 20);
-        int avgB = average(100, 200);
+        int avgA = averageWithOffset(10, 20);
+        int avgB = averageWithOffset(100, 200);
     }
 
-    private static int average(int x, int x1) {
-        return x + x1 / 2;
+    private static int averageWithOffset(int x, int x1) {
+        return x + x1 / 2 - Test.offset - 1;
     }
 }

java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided1.java

@@ -0,0 +1,23 @@
+public class Test {
+
+    void test() {
+        if (<selection>getSize1() < getLimit1() && getCondition1()</selection>) {
+            System.out.println();
+        }
+        if (getSize2() < getLimit2() && getCondition1()) {
+            System.out.println();
+        }
+    }
+
+    int getSize1() {
+      return 42;
+    }
+
+    int getSize2() { return 42; }
+
+    int getLimit1() { return 42; }
+
+    int getLimit2() { return 42; }
+
+    boolean getCondition1 { return true; }
+}

java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided1_after.java

@@ -0,0 +1,27 @@
+public class Test {
+
+    void test() {
+        if (isaBoolean()) {
+            System.out.println();
+        }
+        if (getSize2() < getLimit2() && getCondition1()) {
+            System.out.println();
+        }
+    }
+
+    private boolean isaBoolean() {
+        return getSize1() < getLimit1() && getCondition1();
+    }
+
+    int getSize1() {
+      return 42;
+    }
+
+    int getSize2() { return 42; }
+
+    int getLimit1() { return 42; }
+
+    int getLimit2() { return 42; }
+
+    boolean getCondition1 { return true; }
+}

java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided2.java

@@ -0,0 +1,21 @@
+public class Test {
+
+    void test() {
+        if (<selection>getSize1() < getLimit1()</selection>) {
+            System.out.println();
+        }
+        if (getSize2() < getLimit2()) {
+            System.out.println();
+        }
+    }
+
+    int getSize1() {
+      return 42;
+    }
+
+    int getSize2() { return 42; }
+
+    int getLimit1() { return 42; }
+
+    int getLimit2() { return 42; }
+}

java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided2_after.java

@@ -0,0 +1,25 @@
+public class Test {
+
+    void test() {
+        if (isaBoolean()) {
+            System.out.println();
+        }
+        if (getSize2() < getLimit2()) {
+            System.out.println();
+        }
+    }
+
+    private boolean isaBoolean() {
+        return getSize1() < getLimit1();
+    }
+
+    int getSize1() {
+      return 42;
+    }
+
+    int getSize2() { return 42; }
+
+    int getLimit1() { return 42; }
+
+    int getLimit2() { return 42; }
+}

java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided3.java

@@ -0,0 +1,20 @@
+public class Test {
+
+    boolean test1() {
+        <selection>return getSize1() < getLimit1();</selection>
+    }
+
+    boolean test2() {
+      return getSize2() < getLimit2();
+    }
+
+    int getSize1() {
+      return 42;
+    }
+
+    int getSize2() { return 42; }
+
+    int getLimit1() { return 42; }
+
+    int getLimit2() { return 42; }
+}

java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided3_after.java

@@ -0,0 +1,24 @@
+public class Test {
+
+    boolean test1() {
+        return extracted();
+    }
+
+    private boolean extracted() {
+        return getSize1() < getLimit1();
+    }
+
+    boolean test2() {
+      return getSize2() < getLimit2();
+    }
+
+    int getSize1() {
+      return 42;
+    }
+
+    int getSize2() { return 42; }
+
+    int getLimit1() { return 42; }
+
+    int getLimit2() { return 42; }
+}

java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsNotAvoided.java

@@ -0,0 +1,15 @@
+public class Test {
+
+    void test() {
+        <selection>if (isGood() && isApplicable()) {
+            System.out.println();
+        }</selection>
+        if (!isGood() && isApplicable()) {
+            System.out.println();
+        }
+    }
+
+    boolean isGood() { return true; }
+
+    boolean isApplicable() { return true; }
+}

java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsNotAvoided_after.java

@@ -0,0 +1,17 @@
+public class Test {
+
+    void test() {
+        extracted(isGood());
+        extracted(!isGood());
+    }
+
+    private void extracted(boolean Good) {
+        if (Good && isApplicable()) {
+            System.out.println();
+        }
+    }
+
+    boolean isGood() { return true; }
+
+    boolean isApplicable() { return true; }
+}

java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodAndDuplicatesInplaceTest.kt

@@ -63,7 +63,7 @@ class ExtractMethodAndDuplicatesInplaceTest: LightJavaCodeInsightTestCase() {
   }
 
   fun testRenamedParametrizedDuplicate(){
-    doTest(changedName = "average")
+    doTest(changedName = "averageWithOffset")
   }
 
   fun testStaticMustBePlaced(){
@@ -222,6 +222,22 @@ class ExtractMethodAndDuplicatesInplaceTest: LightJavaCodeInsightTestCase() {
     doTest(changedName = "renamed")
   }
 
+  fun testSignatureChangeIsNotAvoided() {
+    doTest()
+  }
+
+  fun testSignatureChangeIsAvoided1(){
+    doTest()
+  }
+
+  fun testSignatureChangeIsAvoided2(){
+    doTest()
+  }
+
+  fun testSignatureChangeIsAvoided3(){
+    doTest()
+  }
+
   fun testRefactoringListener(){
     templateTest {
       configureByFile("$BASE_PATH/${getTestName(false)}.java")