From f3bfbb473fe4d4c870e5545e9caed64aae6395a0 Mon Sep 17 00:00:00 2001 From: Pavel Dolgov Date: Mon, 9 Oct 2017 15:45:12 +0300 Subject: [PATCH] Java: Extract method from duplicates where some of parameters are hard-coded (IDEA-180092) --- .../refactoring/util/VariableData.java | 12 ++++ .../duplicates/ExtractableExpressionPart.java | 25 ++++++-- .../util/duplicates/ExtractedParameter.java | 22 +++++-- .../extractMethod/ExtractMethodProcessor.java | 2 +- .../JavaDuplicatesExtractMethodProcessor.java | 5 +- .../extractMethod/ParametrizedDuplicates.java | 60 +++++++++++++------ ...atureFourOccurrencesTwoLiteralFolding.java | 23 +++++++ ...ourOccurrencesTwoLiteralFolding_after.java | 24 ++++++++ ...tureFourOccurrencesTwoVariableFolding.java | 27 +++++++++ ...urOccurrencesTwoVariableFolding_after.java | 30 ++++++++++ ...tureThreeOccurrencesTwoLiteralFolding.java | 18 ++++++ ...reeOccurrencesTwoLiteralFolding_after.java | 20 +++++++ ...ureThreeOccurrencesTwoVariableFolding.java | 21 +++++++ ...eeOccurrencesTwoVariableFolding_after.java | 25 ++++++++ .../java/refactoring/ExtractMethodTest.java | 16 +++++ 15 files changed, 299 insertions(+), 31 deletions(-) create mode 100644 java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureFourOccurrencesTwoLiteralFolding.java create mode 100644 java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureFourOccurrencesTwoLiteralFolding_after.java create mode 100644 java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureFourOccurrencesTwoVariableFolding.java create mode 100644 java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureFourOccurrencesTwoVariableFolding_after.java create mode 100644 java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureThreeOccurrencesTwoLiteralFolding.java create mode 100644 java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureThreeOccurrencesTwoLiteralFolding_after.java create mode 100644 java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureThreeOccurrencesTwoVariableFolding.java create mode 100644 java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureThreeOccurrencesTwoVariableFolding_after.java diff --git a/java/java-analysis-impl/src/com/intellij/refactoring/util/VariableData.java b/java/java-analysis-impl/src/com/intellij/refactoring/util/VariableData.java index 5c7d95416bb1..1acdac88e2b5 100644 --- a/java/java-analysis-impl/src/com/intellij/refactoring/util/VariableData.java +++ b/java/java-analysis-impl/src/com/intellij/refactoring/util/VariableData.java @@ -44,4 +44,16 @@ public class VariableData extends AbstractVariableData { this.type = type; } } + + @NotNull + public VariableData substitute(@Nullable PsiVariable var) { + if (var == null) { + return this; + } + VariableData data = new VariableData(var, type); + data.name = name; + data.originalName = originalName; + data.passAsParameter = passAsParameter; + return data; + } } diff --git a/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/ExtractableExpressionPart.java b/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/ExtractableExpressionPart.java index 957283d95f9a..e9e09e4600c1 100644 --- a/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/ExtractableExpressionPart.java +++ b/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/ExtractableExpressionPart.java @@ -56,6 +56,17 @@ public class ExtractableExpressionPart { return null; } + @NotNull + ExtractableExpressionPart copy() { + return new ExtractableExpressionPart(myUsage, myVariable, myValue, myType); + } + + @NotNull + ExtractableExpressionPart deepCopy() { + PsiElementFactory factory = JavaPsiFacade.getElementFactory(myUsage.getProject()); + PsiExpression usageCopy = factory.createExpressionFromText(myUsage.getText(), myUsage); + return new ExtractableExpressionPart(usageCopy, myVariable, myValue, myType); + } boolean isEquivalent(@NotNull ExtractableExpressionPart part) { if (myVariable != null && myVariable.equals(part.myVariable)) { @@ -84,10 +95,7 @@ public class ExtractableExpressionPart { } } if (complexityHolder != null && (isConstant || complexityHolder.isAcceptableExpression(expression))) { - PsiType type = expression.getType(); - if (type != null && !PsiType.VOID.equals(type)) { - return new ExtractableExpressionPart(expression, null, null, type); - } + return matchExpression(expression); } return null; } @@ -115,6 +123,15 @@ public class ExtractableExpressionPart { return null; } + @Nullable + private static ExtractableExpressionPart matchExpression(@NotNull PsiExpression expression) { + PsiType type = expression.getType(); + if (type != null && !PsiType.VOID.equals(type)) { + return new ExtractableExpressionPart(expression, null, null, type); + } + return null; + } + @NotNull public PsiExpression getUsage() { return myUsage; diff --git a/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/ExtractedParameter.java b/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/ExtractedParameter.java index 7847e5e160d2..4eb924cfde1a 100644 --- a/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/ExtractedParameter.java +++ b/java/java-analysis-impl/src/com/intellij/refactoring/util/duplicates/ExtractedParameter.java @@ -32,7 +32,7 @@ public class ExtractedParameter { @NotNull public final PsiType myType; @NotNull public final ExtractableExpressionPart myPattern; @NotNull public final ExtractableExpressionPart myCandidate; - @NotNull public final Map myUsages = new HashMap<>(); + @NotNull public final Set myPatternUsages = new HashSet<>(); public ExtractedParameter(@NotNull ExtractableExpressionPart patternPart, @NotNull ExtractableExpressionPart candidatePart, @@ -40,7 +40,7 @@ public class ExtractedParameter { myType = type; myPattern = patternPart; myCandidate = candidatePart; - addUsages(patternPart, candidatePart); + addUsages(patternPart); } public static boolean match(@NotNull ExtractableExpressionPart patternPart, @@ -60,7 +60,7 @@ public class ExtractedParameter { boolean samePattern = parameter.samePattern(patternPart); boolean sameCandidate = parameter.sameCandidate(candidatePart); if (samePattern && sameCandidate) { - parameter.addUsages(patternPart, candidatePart); + parameter.addUsages(patternPart); return true; } if (samePattern || sameCandidate) { @@ -71,8 +71,20 @@ public class ExtractedParameter { return true; } - private void addUsages(ExtractableExpressionPart patternPart, ExtractableExpressionPart candidatePart) { - myUsages.put(patternPart.getUsage(), candidatePart.getUsage()); + @NotNull + public ExtractedParameter mapPatternToItself(@NotNull Match match) { + ExtractableExpressionPart copy = myPattern.copy(); + ExtractableExpressionPart deepCopy = myPattern.deepCopy(); + + ExtractedParameter parameter = new ExtractedParameter(copy, deepCopy, copy.myType); + parameter.myPatternUsages.addAll(myPatternUsages); + + match.getExtractedParameters().add(parameter); + return parameter; + } + + private void addUsages(ExtractableExpressionPart patternPart) { + myPatternUsages.add(patternPart.getUsage()); } private boolean sameCandidate(ExtractableExpressionPart part) { diff --git a/java/java-impl/src/com/intellij/refactoring/extractMethod/ExtractMethodProcessor.java b/java/java-impl/src/com/intellij/refactoring/extractMethod/ExtractMethodProcessor.java index 5586aa5cb1ec..0ff5fba96ba8 100644 --- a/java/java-impl/src/com/intellij/refactoring/extractMethod/ExtractMethodProcessor.java +++ b/java/java-impl/src/com/intellij/refactoring/extractMethod/ExtractMethodProcessor.java @@ -1972,7 +1972,7 @@ public class ExtractMethodProcessor implements MatchProvider { myExtractedMethod = myParametrizedDuplicates.replaceMethod(myExtractedMethod); myMethodCall = myParametrizedDuplicates.replaceCall(myMethodCall); }); - myVariableDatum = myParametrizedDuplicates.getVariableData(); + myVariableDatum = myParametrizedDuplicates.getVariableDatum(); return null; } } diff --git a/java/java-impl/src/com/intellij/refactoring/extractMethod/JavaDuplicatesExtractMethodProcessor.java b/java/java-impl/src/com/intellij/refactoring/extractMethod/JavaDuplicatesExtractMethodProcessor.java index 47ceae061fbe..1983b4975c0d 100644 --- a/java/java-impl/src/com/intellij/refactoring/extractMethod/JavaDuplicatesExtractMethodProcessor.java +++ b/java/java-impl/src/com/intellij/refactoring/extractMethod/JavaDuplicatesExtractMethodProcessor.java @@ -50,10 +50,7 @@ public class JavaDuplicatesExtractMethodProcessor extends ExtractMethodProcessor VariableData fromData = from.myVariableDatum[i]; PsiVariable mappedVariable = variablesMapping.get(fromData.variable); if (isReferenced(mappedVariable, fromData.variable)) { - VariableData newData = new VariableData(mappedVariable, fromData.type); - newData.name = fromData.name; - newData.originalName = fromData.originalName; - newData.passAsParameter = fromData.passAsParameter; + VariableData newData = fromData.substitute(mappedVariable); variableDatum.add(newData); } } diff --git a/java/java-impl/src/com/intellij/refactoring/extractMethod/ParametrizedDuplicates.java b/java/java-impl/src/com/intellij/refactoring/extractMethod/ParametrizedDuplicates.java index e90a50980e0b..b85896647dab 100644 --- a/java/java-impl/src/com/intellij/refactoring/extractMethod/ParametrizedDuplicates.java +++ b/java/java-impl/src/com/intellij/refactoring/extractMethod/ParametrizedDuplicates.java @@ -36,6 +36,7 @@ import com.intellij.util.containers.ContainerUtil; import com.intellij.util.text.UniqueNameGenerator; import gnu.trove.THashMap; import gnu.trove.THashSet; +import one.util.streamex.StreamEx; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -54,7 +55,7 @@ public class ParametrizedDuplicates { private List myUsagesList; private PsiMethod myParametrizedMethod; private PsiMethodCallExpression myParametrizedCall; - private VariableData[] myVariableData; + private VariableData[] myVariableDatum; private ParametrizedDuplicates(@NotNull PsiElement[] pattern) { LOG.assertTrue(pattern.length != 0, "pattern length"); @@ -158,14 +159,28 @@ public class ParametrizedDuplicates { } } } - myUsagesList.sort(Comparator.comparing(usages -> usages.myFirstOffset)); if (!badMatches.isEmpty()) { matches = new ArrayList<>(matches); matches.removeAll(badMatches); } myMatches = matches; - return !myMatches.isEmpty() && !myUsagesList.isEmpty(); + if (myMatches.isEmpty() || myUsagesList.isEmpty()) { + return false; + } + + for (ClusterOfUsages usages : myUsagesList) { + for (Match match : myMatches) { + ExtractedParameter parameter = usages.myParameters.get(match); + if (parameter == null) { + parameter = usages.myParameter.mapPatternToItself(match); + usages.putParameter(match, parameter); + } + } + } + + myUsagesList.sort(Comparator.comparing(usages -> usages.myFirstOffset)); + return true; } @Nullable @@ -181,7 +196,7 @@ public class ParametrizedDuplicates { if (usages == null) { result.add(usages = new ClusterOfUsages(parameter)); } - usages.add(parameter); + usages.putParameter(match, parameter); } return result; } @@ -208,12 +223,21 @@ public class ParametrizedDuplicates { parametrizedProcessor.setDataFromInputVariables(); myParametrizedMethod = parametrizedProcessor.getExtractedMethod(); myParametrizedCall = parametrizedProcessor.getMethodCall(); - myVariableData = parametrizedProcessor.myVariableDatum; + myVariableDatum = unmapVariableData(parametrizedProcessor.myVariableDatum, variablesMapping); replaceArguments(parameterDeclarations, myParametrizedCall); return true; } + @NotNull + private static VariableData[] unmapVariableData(@NotNull VariableData[] variableDatum, + @NotNull Map variablesMapping) { + Map reverseMapping = ContainerUtil.reverseMap(variablesMapping); + return StreamEx.of(variableDatum) + .map(data -> data.substitute(reverseMapping.get(data.variable))) + .toArray(VariableData[]::new); + } + private static void replaceArguments(@NotNull Map parameterDeclarations, @NotNull PsiMethodCallExpression parametrizedCall) { PsiExpression[] arguments = parametrizedCall.getArgumentList().getExpressions(); @@ -233,7 +257,7 @@ public class ParametrizedDuplicates { private void putMatchParameters(@NotNull Map parameterDeclarations) { Map patternUsageToParameter = new THashMap<>(); for (Map.Entry entry : parameterDeclarations.entrySet()) { - PsiExpression usage = entry.getValue().myParameters.get(0).myPattern.getUsage(); + PsiExpression usage = entry.getValue().myParameter.myPattern.getUsage(); patternUsageToParameter.put(usage, entry.getKey()); } @@ -257,8 +281,8 @@ public class ParametrizedDuplicates { return myParametrizedCall; } - public VariableData[] getVariableData() { - return myVariableData; + public VariableData[] getVariableDatum() { + return myVariableDatum; } public int getSize() { @@ -347,7 +371,7 @@ public class ParametrizedDuplicates { LOG.assertTrue(parent instanceof PsiCodeBlock, "first statement's parent isn't a code block"); for (ClusterOfUsages usages : myUsagesList) { - ExtractedParameter parameter = usages.myParameters.get(0); + ExtractedParameter parameter = usages.myParameter; PsiExpression patternUsage = parameter.myPattern.getUsage(); String initializerText = patternUsage.getText(); PsiExpression initializer = factory.createExpressionFromText(initializerText, parent); @@ -361,7 +385,7 @@ public class ParametrizedDuplicates { PsiLocalVariable localVariable = (PsiLocalVariable)paramDeclaration.getDeclaredElements()[0]; parameterDeclarations.put(localVariable, usages); - for (PsiExpression expression : parameter.myUsages.keySet()) { + for (PsiExpression expression : parameter.myPatternUsages) { PsiExpression mapped = expressionsMapping.get(expression); if (mapped != null) { PsiExpression replacement = factory.createExpressionFromText(parameterName, expression); @@ -444,25 +468,27 @@ public class ParametrizedDuplicates { private static class ClusterOfUsages { @NotNull private final Set myPatterns; - @NotNull private final List myParameters; + @NotNull private final Map myParameters; + @NotNull private final ExtractedParameter myParameter; private final int myFirstOffset; public ClusterOfUsages(@NotNull ExtractedParameter parameter) { - myPatterns = parameter.myUsages.keySet(); - myParameters = new ArrayList<>(); + myPatterns = parameter.myPatternUsages; + myParameters = new THashMap<>(); + myParameter = parameter; myFirstOffset = myPatterns.stream().mapToInt(PsiElement::getTextOffset).min().orElse(0); } - public void add(ExtractedParameter parameter) { - myParameters.add(parameter); + public void putParameter(Match match, ExtractedParameter parameter) { + myParameters.put(match, parameter); } public boolean isEquivalent(ExtractedParameter parameter) { - return myPatterns.equals(parameter.myUsages.keySet()); + return myPatterns.equals(parameter.myPatternUsages); } public static boolean isPresent(Map usagesMap, @NotNull ExtractedParameter parameter) { - return parameter.myUsages.keySet().stream().anyMatch(expression -> usagesMap.get(expression) != null); + return parameter.myPatternUsages.stream().anyMatch(usagesMap::containsKey); } } } diff --git a/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureFourOccurrencesTwoLiteralFolding.java b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureFourOccurrencesTwoLiteralFolding.java new file mode 100644 index 000000000000..1a37881c594f --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureFourOccurrencesTwoLiteralFolding.java @@ -0,0 +1,23 @@ +public class C { + void second() { + test(0); + test(1); + } + + void none() { + test(0); + test(0); + } + + void both() { + test(1); + test(1); + } + + void first() { + test(1); + test(0); + } + + private void test(int i) {} +} diff --git a/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureFourOccurrencesTwoLiteralFolding_after.java b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureFourOccurrencesTwoLiteralFolding_after.java new file mode 100644 index 000000000000..74791e9410e1 --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureFourOccurrencesTwoLiteralFolding_after.java @@ -0,0 +1,24 @@ +public class C { + void second() { + newMethod(0, 1); + } + + private void newMethod(int i, int i2) { + test(i); + test(i2); + } + + void none() { + newMethod(0, 0); + } + + void both() { + newMethod(1, 1); + } + + void first() { + newMethod(1, 0); + } + + private void test(int i) {} +} diff --git a/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureFourOccurrencesTwoVariableFolding.java b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureFourOccurrencesTwoVariableFolding.java new file mode 100644 index 000000000000..69779cf140fc --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureFourOccurrencesTwoVariableFolding.java @@ -0,0 +1,27 @@ +public class C { + void both() { + int x=0,y=0; + a(x);b(y); + a(y);b(x); + } + + void first() { + int k=0; + a(k);b(1); + a(1);b(k); + } + + void second() { + int m=1; + a(0);b(m); + a(m);b(0); + } + + void baz(int e, int f) { + a(e);b(f); + a(f);b(e); + } + + private void a(int i) {} + private void b(int n) {} +} diff --git a/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureFourOccurrencesTwoVariableFolding_after.java b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureFourOccurrencesTwoVariableFolding_after.java new file mode 100644 index 000000000000..323981afddcc --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureFourOccurrencesTwoVariableFolding_after.java @@ -0,0 +1,30 @@ +public class C { + void both() { + int x=0,y=0; + newMethod(y, x); + } + + void first() { + int k=0; + newMethod(1, k); + } + + void second() { + int m=1; + newMethod(m, 0); + } + + private void newMethod(int m, int i) { + a(i); + b(m); + a(m); + b(i); + } + + void baz(int e, int f) { + newMethod(f, e); + } + + private void a(int i) {} + private void b(int n) {} +} diff --git a/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureThreeOccurrencesTwoLiteralFolding.java b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureThreeOccurrencesTwoLiteralFolding.java new file mode 100644 index 000000000000..614473b551a8 --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureThreeOccurrencesTwoLiteralFolding.java @@ -0,0 +1,18 @@ +public class C { + void second() { + test(0); + test(1); + } + + void none() { + test(0); + test(0); + } + + void both() { + test(1); + test(1); + } + + private void test(int i) {} +} diff --git a/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureThreeOccurrencesTwoLiteralFolding_after.java b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureThreeOccurrencesTwoLiteralFolding_after.java new file mode 100644 index 000000000000..78aa719b7610 --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureThreeOccurrencesTwoLiteralFolding_after.java @@ -0,0 +1,20 @@ +public class C { + void second() { + newMethod(0, 1); + } + + private void newMethod(int i, int i2) { + test(i); + test(i2); + } + + void none() { + newMethod(0, 0); + } + + void both() { + newMethod(1, 1); + } + + private void test(int i) {} +} diff --git a/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureThreeOccurrencesTwoVariableFolding.java b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureThreeOccurrencesTwoVariableFolding.java new file mode 100644 index 000000000000..b59032f4aa49 --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureThreeOccurrencesTwoVariableFolding.java @@ -0,0 +1,21 @@ +public class C { + void foo() { + int x=0,y=0; + a(x);b(y); + a(y);b(x); + } + + void bar() { + int k=0,m=1; + a(k);b(m); + a(m);b(k); + } + + void baz(int e, int f) { + a(e);b(f); + a(f);b(e); + } + + private void a(int i) {} + private void b(int n) {} +} diff --git a/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureThreeOccurrencesTwoVariableFolding_after.java b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureThreeOccurrencesTwoVariableFolding_after.java new file mode 100644 index 000000000000..92076be7ae14 --- /dev/null +++ b/java/java-tests/testData/refactoring/extractMethod/SuggestChangeSignatureThreeOccurrencesTwoVariableFolding_after.java @@ -0,0 +1,25 @@ +public class C { + void foo() { + int x=0,y=0; + newMethod(x, y); + } + + private void newMethod(int x, int y) { + a(x); + b(y); + a(y); + b(x); + } + + void bar() { + int k=0,m=1; + newMethod(k, m); + } + + void baz(int e, int f) { + newMethod(e, f); + } + + private void a(int i) {} + private void b(int n) {} +} diff --git a/java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodTest.java b/java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodTest.java index d7224c42aaed..fd2fc12f4c8b 100644 --- a/java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodTest.java +++ b/java/java-tests/testSrc/com/intellij/java/refactoring/ExtractMethodTest.java @@ -811,6 +811,22 @@ public class ExtractMethodTest extends LightCodeInsightTestCase { doDuplicatesTest(); } + public void testSuggestChangeSignatureThreeOccurrencesTwoLiteralFolding() throws Exception { + doDuplicatesTest(); + } + + public void testSuggestChangeSignatureFourOccurrencesTwoLiteralFolding() throws Exception { + doDuplicatesTest(); + } + + public void testSuggestChangeSignatureThreeOccurrencesTwoVariableFolding() throws Exception { + doDuplicatesTest(); + } + + public void testSuggestChangeSignatureFourOccurrencesTwoVariableFolding() throws Exception { + doDuplicatesTest(); + } + public void testSuggestChangeSignatureWithOutputVariables() throws Exception { doDuplicatesTest(); }