From 73f35ed28cdd7cc536ccd3624d146bd93f80a052 Mon Sep 17 00:00:00 2001 From: Alexandr Suhinin Date: Thu, 10 Nov 2022 01:59:23 +0200 Subject: [PATCH] IDEA-274123 [extract method]: don't suggest too abstract change signatures GitOrigin-RevId: 4c59710e2304288ad4465a92ec36bff633bd661e --- .../inplace/DuplicatesMethodExtractor.kt | 19 ++++++++++++- .../DuplicatedButDeclined_after.java | 6 ++--- ...uplicatedExpressionAndChangeSignature.java | 8 +++--- ...tedExpressionAndChangeSignature_after.java | 12 ++++----- .../RenamedParametrizedDuplicate.java | 6 +++-- .../RenamedParametrizedDuplicate_after.java | 10 ++++--- .../SignatureChangeIsAvoided1.java | 23 ++++++++++++++++ .../SignatureChangeIsAvoided1_after.java | 27 +++++++++++++++++++ .../SignatureChangeIsAvoided2.java | 21 +++++++++++++++ .../SignatureChangeIsAvoided2_after.java | 25 +++++++++++++++++ .../SignatureChangeIsAvoided3.java | 20 ++++++++++++++ .../SignatureChangeIsAvoided3_after.java | 24 +++++++++++++++++ .../SignatureChangeIsNotAvoided.java | 15 +++++++++++ .../SignatureChangeIsNotAvoided_after.java | 17 ++++++++++++ .../ExtractMethodAndDuplicatesInplaceTest.kt | 18 ++++++++++++- 15 files changed, 230 insertions(+), 21 deletions(-) create mode 100644 java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided1.java create mode 100644 java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided1_after.java create mode 100644 java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided2.java create mode 100644 java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided2_after.java create mode 100644 java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided3.java create mode 100644 java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided3_after.java create mode 100644 java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsNotAvoided.java create mode 100644 java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsNotAvoided_after.java diff --git a/java/java-impl-refactorings/src/com/intellij/refactoring/extractMethod/newImpl/inplace/DuplicatesMethodExtractor.kt b/java/java-impl-refactorings/src/com/intellij/refactoring/extractMethod/newImpl/inplace/DuplicatesMethodExtractor.kt index 4afb9da91644..75863638d099 100644 --- a/java/java-impl-refactorings/src/com/intellij/refactoring/extractMethod/newImpl/inplace/DuplicatesMethodExtractor.kt +++ b/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, initialParameters: List, + callAfter: List, updatedParameters: List): 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) } diff --git a/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/DuplicatedButDeclined_after.java b/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/DuplicatedButDeclined_after.java index 8d4913c134c7..58f080849431 100644 --- a/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/DuplicatedButDeclined_after.java +++ b/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) { diff --git a/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/DuplicatedExpressionAndChangeSignature.java b/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/DuplicatedExpressionAndChangeSignature.java index a8a7156e87ff..ab13fad3ef8a 100644 --- a/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/DuplicatedExpressionAndChangeSignature.java +++ b/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/DuplicatedExpressionAndChangeSignature.java @@ -1,6 +1,6 @@ class Test { - public void test() { - byte[] one = "one".getBytes(); - byte[] two = "two".getBytes(); - } + public void test() { + String start1 = "one".substring(0, 10); + String start2 = "two".substring(0, 10); + } } \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/DuplicatedExpressionAndChangeSignature_after.java b/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/DuplicatedExpressionAndChangeSignature_after.java index 5b7583107a44..dc78bbe0c610 100644 --- a/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/DuplicatedExpressionAndChangeSignature_after.java +++ b/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); } } \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/RenamedParametrizedDuplicate.java b/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/RenamedParametrizedDuplicate.java index a27c4d3e59c6..ebeb7664a77e 100644 --- a/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/RenamedParametrizedDuplicate.java +++ b/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/RenamedParametrizedDuplicate.java @@ -1,6 +1,8 @@ class Test { + static int offset = 42; + void test(){ - int avgA = 10 + 20 / 2; - int avgB = 100 + 200 / 2; + int avgA = 10 + 20 / 2 - Test.offset - 1; + int avgB = 100 + 200 / 2 - Test.offset - 1; } } \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/RenamedParametrizedDuplicate_after.java b/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/RenamedParametrizedDuplicate_after.java index dd2a895bc42e..91b742fcc8ee 100644 --- a/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/RenamedParametrizedDuplicate_after.java +++ b/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; } } \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided1.java b/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided1.java new file mode 100644 index 000000000000..aaba082992f6 --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided1.java @@ -0,0 +1,23 @@ +public class Test { + + void test() { + if (getSize1() < getLimit1() && getCondition1()) { + 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; } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided1_after.java b/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided1_after.java new file mode 100644 index 000000000000..36a15af2f3c3 --- /dev/null +++ b/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; } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided2.java b/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided2.java new file mode 100644 index 000000000000..d939739de025 --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided2.java @@ -0,0 +1,21 @@ +public class Test { + + void test() { + if (getSize1() < getLimit1()) { + 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; } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided2_after.java b/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided2_after.java new file mode 100644 index 000000000000..40716b085da1 --- /dev/null +++ b/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; } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided3.java b/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided3.java new file mode 100644 index 000000000000..1461193ba637 --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided3.java @@ -0,0 +1,20 @@ +public class Test { + + boolean test1() { + return getSize1() < getLimit1(); + } + + boolean test2() { + return getSize2() < getLimit2(); + } + + int getSize1() { + return 42; + } + + int getSize2() { return 42; } + + int getLimit1() { return 42; } + + int getLimit2() { return 42; } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided3_after.java b/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsAvoided3_after.java new file mode 100644 index 000000000000..5f83bf1f015f --- /dev/null +++ b/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; } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsNotAvoided.java b/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsNotAvoided.java new file mode 100644 index 000000000000..b67cf8dbbb73 --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsNotAvoided.java @@ -0,0 +1,15 @@ +public class Test { + + void test() { + if (isGood() && isApplicable()) { + System.out.println(); + } + if (!isGood() && isApplicable()) { + System.out.println(); + } + } + + boolean isGood() { return true; } + + boolean isApplicable() { return true; } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsNotAvoided_after.java b/java/java-tests/testData/refactoring/extractMethodAndDuplicatesInplace/SignatureChangeIsNotAvoided_after.java new file mode 100644 index 000000000000..099a1ef534fc --- /dev/null +++ b/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; } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodAndDuplicatesInplaceTest.kt b/java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodAndDuplicatesInplaceTest.kt index 73c572672650..95d8f2db834c 100644 --- a/java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodAndDuplicatesInplaceTest.kt +++ b/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")