diff --git a/java/java-impl/src/com/intellij/refactoring/extractclass/ExtractClassProcessor.java b/java/java-impl/src/com/intellij/refactoring/extractclass/ExtractClassProcessor.java index 8b8daed6696b..51244b582106 100644 --- a/java/java-impl/src/com/intellij/refactoring/extractclass/ExtractClassProcessor.java +++ b/java/java-impl/src/com/intellij/refactoring/extractclass/ExtractClassProcessor.java @@ -131,30 +131,25 @@ public class ExtractClassProcessor extends FixableUsagesRefactoringProcessor { if (!myGenerateAccessors) { calculateInitializersConflicts(conflicts); - final NecessaryAccessorsVisitor visitor = new NecessaryAccessorsVisitor(); - for (PsiField field : fields) { - field.accept(visitor); - } - for (PsiMethod method : methods) { - method.accept(visitor); - } - for (PsiClass innerClass : innerClasses) { - innerClass.accept(visitor); - } - - final Set fieldsNeedingGetter = visitor.getFieldsNeedingGetter(); + final NecessaryAccessorsVisitor visitor = checkNecessaryGettersSetters4ExtractedClass(); + final NecessaryAccessorsVisitor srcVisitor = checkNecessaryGettersSetters4SourceClass(); + final Set fieldsNeedingGetter = new LinkedHashSet(); + fieldsNeedingGetter.addAll(visitor.getFieldsNeedingGetter()); + fieldsNeedingGetter.addAll(srcVisitor.getFieldsNeedingGetter()); for (PsiField field : fieldsNeedingGetter) { conflicts.putValue(field, "Field \'" + field.getName() + "\' needs getter"); } - final Set fieldsNeedingSetter = visitor.getFieldsNeedingSetter(); + final Set fieldsNeedingSetter = new LinkedHashSet(); + fieldsNeedingSetter.addAll(visitor.getFieldsNeedingSetter()); + fieldsNeedingSetter.addAll(srcVisitor.getFieldsNeedingSetter()); for (PsiField field : fieldsNeedingSetter) { - conflicts.putValue(field, "Field \'" + field.getName() + "\' needs getter"); + conflicts.putValue(field, "Field \'" + field.getName() + "\' needs setter"); } } return showConflicts(conflicts, refUsages.get()); } - + private void calculateInitializersConflicts(MultiMap conflicts) { final PsiClassInitializer[] initializers = sourceClass.getInitializers(); for (PsiClassInitializer initializer : initializers) { @@ -271,16 +266,7 @@ public class ExtractClassProcessor extends FixableUsagesRefactoringProcessor { } if (myGenerateAccessors) { - final NecessaryAccessorsVisitor visitor = new NecessaryAccessorsVisitor(); - for (PsiField field : fields) { - field.accept(visitor); - } - for (PsiMethod method : methods) { - method.accept(visitor); - } - for (PsiClass innerClass : innerClasses) { - innerClass.accept(visitor); - } + final NecessaryAccessorsVisitor visitor = checkNecessaryGettersSetters4SourceClass(); for (PsiField field : visitor.getFieldsNeedingGetter()) { sourceClass.add(PropertyUtil.generateGetterPrototype(field)); } @@ -296,6 +282,83 @@ public class ExtractClassProcessor extends FixableUsagesRefactoringProcessor { } } + private NecessaryAccessorsVisitor checkNecessaryGettersSetters4SourceClass() { + final NecessaryAccessorsVisitor visitor = new NecessaryAccessorsVisitor() { + @Override + protected boolean hasGetterOrSetter(PsiMethod[] getters) { + for (PsiMethod getter : getters) { + if (!isInMovedElement(getter)) return true; + } + return false; + } + + @Override + protected boolean isProhibitedReference(PsiField field) { + if (fields.contains(field)) { + return false; + } + if (innerClasses.contains(field.getContainingClass())) { + return false; + } + return true; + } + }; + for (PsiField field : fields) { + field.accept(visitor); + } + for (PsiMethod method : methods) { + method.accept(visitor); + } + for (PsiClass innerClass : innerClasses) { + innerClass.accept(visitor); + } + return visitor; + } + + private NecessaryAccessorsVisitor checkNecessaryGettersSetters4ExtractedClass() { + final NecessaryAccessorsVisitor visitor = new NecessaryAccessorsVisitor() { + @Override + protected boolean hasGetterOrSetter(PsiMethod[] getters) { + for (PsiMethod getter : getters) { + if (isInMovedElement(getter)) return true; + } + return false; + } + + @Override + protected boolean isProhibitedReference(PsiField field) { + if (fields.contains(field)) { + return true; + } + if (innerClasses.contains(field.getContainingClass())) { + return true; + } + return false; + } + + @Override + public void visitMethod(PsiMethod method) { + if (methods.contains(method)) return; + super.visitMethod(method); + } + + @Override + public void visitField(PsiField field) { + if (fields.contains(field)) return; + super.visitField(field); + } + + @Override + public void visitClass(PsiClass aClass) { + if (innerClasses.contains(aClass)) return; + super.visitClass(aClass); + } + + }; + sourceClass.accept(visitor); + return visitor; + } + private void buildDelegate() { final PsiManager manager = sourceClass.getManager(); @@ -501,8 +564,25 @@ public class ExtractClassProcessor extends FixableUsagesRefactoringProcessor { final GlobalSearchScope scope = GlobalSearchScope.allScope(project); final String qualifiedName = StringUtil.getQualifiedName(newPackageName, newClassName); - @NonNls final String getter = PropertyUtil.suggestGetterName(myProject, field); - @NonNls final String setter = PropertyUtil.suggestSetterName(myProject, field); + @NonNls String getter = null; + if (myGenerateAccessors) { + getter = PropertyUtil.suggestGetterName(myProject, field); + } else { + final PsiMethod fieldGetter = PropertyUtil.findPropertyGetter(sourceClass, field.getName(), false, false); + if (fieldGetter != null && isInMovedElement(fieldGetter)) { + getter = fieldGetter.getName(); + } + } + + @NonNls String setter = null; + if (myGenerateAccessors) { + setter = PropertyUtil.suggestSetterName(myProject, field); + } else { + final PsiMethod fieldSetter = PropertyUtil.findPropertySetter(sourceClass, field.getName(), false, false); + if (fieldSetter != null && isInMovedElement(fieldSetter)) { + setter = fieldSetter.getName(); + } + } final boolean isStatic = field.hasModifierProperty(PsiModifier.STATIC); for (PsiReference reference : ReferencesSearch.search(field, scope)) { @@ -516,19 +596,19 @@ public class ExtractClassProcessor extends FixableUsagesRefactoringProcessor { if (RefactoringUtil.isPlusPlusOrMinusMinus(exp.getParent())) { usages.add(isStatic ? new ReplaceStaticVariableIncrementDecrement(exp, qualifiedName) - : new ReplaceInstanceVariableIncrementDecrement(exp, delegateFieldName, setter, getter)); + : new ReplaceInstanceVariableIncrementDecrement(exp, delegateFieldName, setter, getter, field.getName())); } else if (RefactoringUtil.isAssignmentLHS(exp)) { usages.add(isStatic ? new ReplaceStaticVariableAssignment(exp, qualifiedName) : new ReplaceInstanceVariableAssignment(PsiTreeUtil.getParentOfType(exp, PsiAssignmentExpression.class), - delegateFieldName, setter, getter)); + delegateFieldName, setter, getter, field.getName())); } else { usages.add(isStatic ? new ReplaceStaticVariableAccess(exp, qualifiedName) - : new ReplaceInstanceVariableAccess(exp, delegateFieldName, getter)); + : new ReplaceInstanceVariableAccess(exp, delegateFieldName, getter, field.getName())); } if (!isStatic) { @@ -540,21 +620,6 @@ public class ExtractClassProcessor extends FixableUsagesRefactoringProcessor { } } - private boolean hasGetter(final PsiField field) { - return hasGetterOrSetter(sourceClass.findMethodsBySignature(PropertyUtil.generateGetterPrototype(field), false)); - } - - private boolean hasSetter(final PsiField field) { - return hasGetterOrSetter(sourceClass.findMethodsBySignature(PropertyUtil.generateSetterPrototype(field), false)); - } - - private boolean hasGetterOrSetter(final PsiMethod[] getters) { - for (PsiMethod getter : getters) { - if (!isInMovedElement(getter)) return true; - } - return false; - } - private PsiClass buildClass() { final PsiManager manager = sourceClass.getManager(); @@ -579,18 +644,7 @@ public class ExtractClassProcessor extends FixableUsagesRefactoringProcessor { extractedClassBuilder.setInterfaces(interfaces); if (myGenerateAccessors) { - final NecessaryAccessorsVisitor visitor = new NecessaryAccessorsVisitor() { - @Override - protected boolean isProhibitedReference(PsiField field) { - if (fields.contains(field)) { - return true; - } - if (innerClasses.contains(field.getContainingClass())) { - return true; - } - return false; - } - }; + final NecessaryAccessorsVisitor visitor = checkNecessaryGettersSetters4ExtractedClass(); sourceClass.accept(visitor); extractedClassBuilder.setFieldsNeedingGetters(visitor.getFieldsNeedingGetter()); extractedClassBuilder.setFieldsNeedingSetters(visitor.getFieldsNeedingSetter()); @@ -716,7 +770,7 @@ public class ExtractClassProcessor extends FixableUsagesRefactoringProcessor { return true; } - private class NecessaryAccessorsVisitor extends JavaRecursiveElementWalkingVisitor { + private abstract class NecessaryAccessorsVisitor extends JavaRecursiveElementWalkingVisitor { private final Set fieldsNeedingGetter = new HashSet(); private final Set fieldsNeedingSetter = new HashSet(); @@ -724,7 +778,7 @@ public class ExtractClassProcessor extends FixableUsagesRefactoringProcessor { super.visitReferenceExpression(expression); if (isProhibitedReference(expression)) { final PsiField field = getReferencedField(expression); - if (!hasGetter(field) && !isStaticFinal(field)) { + if (!hasGetter(field) && !isStaticFinal(field) && !field.getModifierList().hasModifierProperty(PsiModifier.PUBLIC)) { fieldsNeedingGetter.add(field); } } @@ -742,7 +796,7 @@ public class ExtractClassProcessor extends FixableUsagesRefactoringProcessor { final PsiExpression lhs = expression.getLExpression(); if (isProhibitedReference(lhs)) { final PsiField field = getReferencedField(lhs); - if (!hasGetter(field) && !isStaticFinal(field)) { + if (!hasGetter(field) && !isStaticFinal(field) && !field.getModifierList().hasModifierProperty(PsiModifier.PUBLIC)) { fieldsNeedingSetter.add(field); } } @@ -779,6 +833,15 @@ public class ExtractClassProcessor extends FixableUsagesRefactoringProcessor { return fieldsNeedingSetter; } + private boolean hasGetter(final PsiField field) { + return hasGetterOrSetter(sourceClass.findMethodsBySignature(PropertyUtil.generateGetterPrototype(field), false)); + } + + private boolean hasSetter(final PsiField field) { + return hasGetterOrSetter(sourceClass.findMethodsBySignature(PropertyUtil.generateSetterPrototype(field), false)); + } + + protected abstract boolean hasGetterOrSetter(final PsiMethod[] getters); protected boolean isProhibitedReference(PsiExpression expression) { return BackpointerUtil.isBackpointerReference(expression, new Condition() { @@ -788,15 +851,7 @@ public class ExtractClassProcessor extends FixableUsagesRefactoringProcessor { }); } - protected boolean isProhibitedReference(PsiField field) { - if (fields.contains(field)) { - return false; - } - if (innerClasses.contains(field.getContainingClass())) { - return false; - } - return true; - } + protected abstract boolean isProhibitedReference(PsiField field); private PsiField getReferencedField(PsiExpression expression) { if (expression instanceof PsiParenthesizedExpression) { diff --git a/java/java-impl/src/com/intellij/refactoring/extractclass/usageInfo/ReplaceInstanceVariableAccess.java b/java/java-impl/src/com/intellij/refactoring/extractclass/usageInfo/ReplaceInstanceVariableAccess.java index a6452a783fd3..9406473379ea 100644 --- a/java/java-impl/src/com/intellij/refactoring/extractclass/usageInfo/ReplaceInstanceVariableAccess.java +++ b/java/java-impl/src/com/intellij/refactoring/extractclass/usageInfo/ReplaceInstanceVariableAccess.java @@ -23,19 +23,21 @@ import com.intellij.util.IncorrectOperationException; public class ReplaceInstanceVariableAccess extends FixableUsageInfo { private final PsiReferenceExpression expression; + private final String fieldName; private final String getterName; private final String delegateName; - public ReplaceInstanceVariableAccess(PsiReferenceExpression expression, String delegateName, String getterName) { + public ReplaceInstanceVariableAccess(PsiReferenceExpression expression, String delegateName, String getterName, String name) { super(expression); this.getterName = getterName; this.delegateName = delegateName; this.expression = expression; + fieldName = name; } public void fixUsage() throws IncorrectOperationException { final PsiElement qualifier = expression.getQualifier(); - final String callString = delegateName + '.' + getterName + "()"; + final String callString = delegateName + '.' + (getterName != null ? getterName + "()" : fieldName); if (qualifier != null) { final String qualifierText = qualifier.getText(); MutationUtils.replaceExpression(qualifierText + '.' + callString, expression); diff --git a/java/java-impl/src/com/intellij/refactoring/extractclass/usageInfo/ReplaceInstanceVariableAssignment.java b/java/java-impl/src/com/intellij/refactoring/extractclass/usageInfo/ReplaceInstanceVariableAssignment.java index 445ead8872aa..637251e65b84 100644 --- a/java/java-impl/src/com/intellij/refactoring/extractclass/usageInfo/ReplaceInstanceVariableAssignment.java +++ b/java/java-impl/src/com/intellij/refactoring/extractclass/usageInfo/ReplaceInstanceVariableAssignment.java @@ -21,52 +21,74 @@ import com.intellij.refactoring.util.FixableUsageInfo; import com.intellij.util.IncorrectOperationException; public class ReplaceInstanceVariableAssignment extends FixableUsageInfo { - private final String setterName; - private final PsiAssignmentExpression assignment; - private final String getterName; - private final String delegateName; + private final String setterName; + private final PsiAssignmentExpression assignment; + private final String getterName; + private final String delegateName; + private final String fieldName; - public ReplaceInstanceVariableAssignment(PsiAssignmentExpression assignment, - String delegateName, - String setterName, - String getterName) { - super(assignment); - this.assignment = assignment; - this.getterName = getterName; - this.setterName = setterName; - this.delegateName = delegateName; - } + public ReplaceInstanceVariableAssignment(PsiAssignmentExpression assignment, + String delegateName, + String setterName, + String getterName, String name) { + super(assignment); + this.assignment = assignment; + this.getterName = getterName; + this.setterName = setterName; + this.delegateName = delegateName; + fieldName = name; + } - public void fixUsage() throws IncorrectOperationException { - final PsiReferenceExpression lhs = - (PsiReferenceExpression) assignment.getLExpression(); - final PsiExpression rhs = assignment.getRExpression(); - assert rhs != null; - final PsiElement qualifier = lhs.getQualifier(); - final PsiJavaToken sign = assignment.getOperationSign(); - final String operator = sign.getText(); - final String rhsText = rhs.getText(); - final String newExpression; - if (qualifier != null) { - final String qualifierText = qualifier.getText(); - if ("=".equals(operator)) { - newExpression = qualifierText + '.' + delegateName + '.' + setterName + "( " + rhsText + ')'; - } else { - final String strippedOperator = getStrippedOperator(operator); - newExpression = qualifierText + '.'+delegateName + '.' + setterName + '(' + qualifierText + '.'+delegateName + '.' + getterName + "()" + strippedOperator + rhsText + ')'; - } - } else { - if ("=".equals(operator)) { - newExpression = delegateName + '.' + setterName + "( " + rhsText + ')'; - } else { - final String strippedOperator = getStrippedOperator(operator); - newExpression = delegateName + '.' + setterName + '(' + delegateName + '.' + getterName + "()" + strippedOperator + rhsText + ')'; - } - } - MutationUtils.replaceExpression(newExpression, assignment); + public void fixUsage() throws IncorrectOperationException { + final PsiReferenceExpression lhs = + (PsiReferenceExpression)assignment.getLExpression(); + final PsiExpression rhs = assignment.getRExpression(); + assert rhs != null; + final PsiElement qualifier = lhs.getQualifier(); + final PsiJavaToken sign = assignment.getOperationSign(); + final String operator = sign.getText(); + final String rhsText = rhs.getText(); + final String newExpression; + if (qualifier != null) { + final String qualifierText = qualifier.getText(); + if ("=".equals(operator)) { + newExpression = qualifierText + '.' + delegateName + '.' + callSetter(rhsText); + } + else { + final String strippedOperator = getStrippedOperator(operator); + newExpression = qualifierText + + '.' + + delegateName + + '.' + + callSetter( + qualifierText + + '.' + + delegateName + + '.' + + callGetter() + strippedOperator + rhsText); + } } + else { + if ("=".equals(operator)) { + newExpression = delegateName + '.' + callSetter(rhsText); + } + else { + final String strippedOperator = getStrippedOperator(operator); + newExpression = delegateName + '.' + callSetter(delegateName + '.' + callGetter() + strippedOperator + rhsText); + } + } + MutationUtils.replaceExpression(newExpression, assignment); + } - private static String getStrippedOperator(String operator) { - return operator.substring(0, operator.length() - 1); - } + private String callSetter(String rhsText) { + return setterName != null ? setterName + "( " + rhsText + ")" : fieldName + "=" + rhsText; + } + + private String callGetter() { + return getterName != null ? getterName + "()" : fieldName; + } + + private static String getStrippedOperator(String operator) { + return operator.substring(0, operator.length() - 1); + } } diff --git a/java/java-impl/src/com/intellij/refactoring/extractclass/usageInfo/ReplaceInstanceVariableIncrementDecrement.java b/java/java-impl/src/com/intellij/refactoring/extractclass/usageInfo/ReplaceInstanceVariableIncrementDecrement.java index 8d4fc1fddd35..c6417d6f9faf 100644 --- a/java/java-impl/src/com/intellij/refactoring/extractclass/usageInfo/ReplaceInstanceVariableIncrementDecrement.java +++ b/java/java-impl/src/com/intellij/refactoring/extractclass/usageInfo/ReplaceInstanceVariableIncrementDecrement.java @@ -20,18 +20,25 @@ import com.intellij.psi.util.PsiTreeUtil; import com.intellij.refactoring.psi.MutationUtils; import com.intellij.refactoring.util.FixableUsageInfo; import com.intellij.util.IncorrectOperationException; +import org.jetbrains.annotations.Nullable; public class ReplaceInstanceVariableIncrementDecrement extends FixableUsageInfo { private final PsiExpression reference; - private final String setterName; - private final String getterName; + private final @Nullable String setterName; + private final @Nullable String getterName; private final String delegateName; + private final String fieldName; - public ReplaceInstanceVariableIncrementDecrement(PsiExpression reference, String delegateName, String setterName, String getterName) { + public ReplaceInstanceVariableIncrementDecrement(PsiExpression reference, + String delegateName, + String setterName, + String getterName, + String name) { super(reference); this.getterName = getterName; this.setterName = setterName; this.delegateName = delegateName; + fieldName = name; final PsiPrefixExpression prefixExpr = PsiTreeUtil.getParentOfType(reference, PsiPrefixExpression.class); if (prefixExpr != null) { this.reference = prefixExpr; @@ -56,31 +63,26 @@ public class ReplaceInstanceVariableIncrementDecrement extends FixableUsageInfo final PsiElement qualifier = lhs.getQualifier(); final String operator = sign.getText(); final String newExpression; + final String strippedOperator = getStrippedOperator(operator); if (qualifier != null) { final String qualifierText = qualifier.getText(); - final String strippedOperator = getStrippedOperator(operator); - newExpression = qualifierText + - '.' + - delegateName + - '.' + - setterName + - '(' + - qualifierText + - '.' + - delegateName + - '.' + - getterName + - "()" + - strippedOperator + - "1)"; + newExpression = qualifierText + '.' + delegateName + '.' + + callSetter(qualifierText + '.' + delegateName + '.' + callGetter() + strippedOperator + "1"); } else { - final String strippedOperator = getStrippedOperator(operator); - newExpression = delegateName + '.' + setterName + '(' + delegateName + '.' + getterName + "()" + strippedOperator + "1)"; + newExpression = delegateName + '.' + callSetter(delegateName + '.' + callGetter() + strippedOperator + "1"); } MutationUtils.replaceExpression(newExpression, reference); } + private String callGetter() { + return (getterName != null ? getterName + "()" : fieldName); + } + + private String callSetter(String rhsText) { + return setterName != null ? setterName + "(" + rhsText + ")" : fieldName + "=" + rhsText; + } + private static String getStrippedOperator(String operator) { return operator.substring(0, operator.length() - 1); } diff --git a/java/java-tests/testData/refactoring/extractClass/multipleGetters/after/Extracted.java b/java/java-tests/testData/refactoring/extractClass/multipleGetters/after/Extracted.java deleted file mode 100644 index 8d1954100f73..000000000000 --- a/java/java-tests/testData/refactoring/extractClass/multipleGetters/after/Extracted.java +++ /dev/null @@ -1,6 +0,0 @@ -public class Extracted { - int myT; - - public Extracted() { - } -} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractClass/multipleGetters/after/Test.java b/java/java-tests/testData/refactoring/extractClass/multipleGetters/after/Test.java index d5b28e57d91e..7ae85c85a278 100644 --- a/java/java-tests/testData/refactoring/extractClass/multipleGetters/after/Test.java +++ b/java/java-tests/testData/refactoring/extractClass/multipleGetters/after/Test.java @@ -1,10 +1,9 @@ class Test { - final Extracted extracted = new Extracted(); - - public int getMyT() { - return extracted.getMyT(); + int myT; + public int getMyT() { + return myT; } void bar(){ - int i = extracted.getMyT(); + int i = myT; } } \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/extractClass/publicFieldDelegation/after/Test.java b/java/java-tests/testData/refactoring/extractClass/publicFieldDelegation/after/Test.java index adfd3779e3d6..61c59fccd50c 100644 --- a/java/java-tests/testData/refactoring/extractClass/publicFieldDelegation/after/Test.java +++ b/java/java-tests/testData/refactoring/extractClass/publicFieldDelegation/after/Test.java @@ -3,6 +3,6 @@ class Test { void foo(T t){} void bar(){ - foo(extracted.getMyT()); + foo(extracted.myT); } } \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/refactoring/ExtractClassTest.java b/java/java-tests/testSrc/com/intellij/refactoring/ExtractClassTest.java index f060901f2d58..366341325dc9 100644 --- a/java/java-tests/testSrc/com/intellij/refactoring/ExtractClassTest.java +++ b/java/java-tests/testSrc/com/intellij/refactoring/ExtractClassTest.java @@ -17,6 +17,8 @@ import com.intellij.refactoring.extractclass.ExtractClassProcessor; import junit.framework.Assert; import java.util.ArrayList; +import java.util.Arrays; +import java.util.TreeSet; public class ExtractClassTest extends MultiFileTestCase{ protected String getTestRoot() { @@ -140,7 +142,9 @@ public class ExtractClassTest extends MultiFileTestCase{ } catch (BaseRefactoringProcessor.ConflictsInTestsException e) { if (conflicts != null) { - Assert.assertEquals(e.getMessage(), conflicts); + TreeSet expectedConflictsSet = new TreeSet(Arrays.asList(conflicts.split("\n"))); + TreeSet actualConflictsSet = new TreeSet(Arrays.asList(e.getMessage().split("\n"))); + Assert.assertEquals(expectedConflictsSet, actualConflictsSet); return; } else { fail(e.getMessage()); @@ -208,7 +212,7 @@ public class ExtractClassTest extends MultiFileTestCase{ } public void testMultipleGetters() throws Exception { - doTestField(null); + doTestField("Field 'myT' needs getter"); } public void testMultipleGetters1() throws Exception { @@ -216,11 +220,15 @@ public class ExtractClassTest extends MultiFileTestCase{ } public void testUsedInInitializer() throws Exception { - doTestField("Class initializer requires moved members"); + doTestField("Field 'myT' needs setter\n" + + "Field 'myT' needs getter\n" + + "Class initializer requires moved members"); } public void testUsedInConstructor() throws Exception { - doTestField("Constructor requires moved members"); + doTestField("Field 'myT' needs getter\n" + + "Field 'myT' needs setter\n" + + "Constructor requires moved members"); } public void testRefInJavadoc() throws Exception {