Community
Participate
Working Groups
feedback from thoughtworks: ---- I had a set of 3 co-dependant methods on a class and I wanted to move that functionality off into an external static class. I had to manually make them all static, and then do a Refactor-Move on each method one by one. However, this actually didn't work because the first method was private, and the refactoring didn't solve it. In the end I fixed most of it manually. ---- We should provide the same support for dependent methods as we do in pull-up
Created attachment 5832 [details] Fix 1 OK, this is only a small first step ... - Take this class: package codep; public class A { private static int fCount; public static int getCount() { return fCount; } } - Try to move fCount to some other class, click Preview> - A dialog warns about possible errors. Click <Back - Click Preview> again ... Boom! The problem was that MoveStaticMethodRefactoring#checkInput didn't expect to be called more than once.
This is a test class: public class A { private static int fCount; private static void setCount(int count) { fCount= count; } private static void incCount(int inc) { setCount(getCount() + inc); } private static int internalGetCount() { return fCount;} public static int getCount() { return internalGetCount(); } public int nonstaticGetCount() { return getCount(); } private static int fIndependent; } Selecting the first 5 members in the Outline and calling Refactor>Move works. However, clicking "Preview" shows some warnings that moved members will not be visible after move (which doesn't hinder me to press "Continue"). Providing "the same functionality as for pull up" would mean to show a big dialog with all static members and opportunity to move members or to increase their visibility. This is IMO unrequired UI bloat. The attached Fix 1 is still valid.
Release the patch. Markus, I agree with our comment 2. Can you please confirm that if we move to private method that reference each other we don't see any warning. If we do, can you provide a patch.
Created attachment 5925 [details] Fix 2 Fix 2 adds more thorough visibility checks. Warning messages now appear only if references from outside of the moved members will cause visibility errors. test21 covered this case. It revealed another issue which is also fixed: The order of static members is important for fields. Field initializers can only referenced *after* they are defined. The fix preserves the order of moved members. I couldn't find out what was wrong with test22 (except for a typo), so I reactivated it.
Created attachment 5926 [details] Tests for Fix 2
Markus, why do you do the sorting in check activation ? I think it can be moved to checkInput.
I was afraid of having different orderings in fMembersToMove and fMemberBindings. But there doesn't seem to be a direct relationship between them, so we can put it into checkActivation().
Created attachment 5954 [details] Fix 3 This patch contains fixes for several bugs: Bug 37756: - Better visibility checks: All real problems should now be detected and flagged as ERROR. There are no WARNING messages any more. Improved message text includes visibility status of enclosing type, which could as well be the reason for visibility errors. Bug 28022: - Enabled moving "public static final" fields between classes and interfaces (with appropriate error checking) Bug 42383: - Moving field declarations with multiple variables is now FATAL. I had to move the invocation of getASTMembers() into checkActivation(), because we need FieldDeclarations from the AST to find those which declare multiple variables. Sorting is still done inside checkActivation(), but that's less of an efficiency problem now, since it uses getStartPosition() from the AST rather than from JavaElements. Others: - Solved problems with ! workingCopy.equals(primary) causing some invocations to fail (e.g. when the target class contained references to moving members).
Created attachment 5955 [details] Tests for Fix 3
Released the patch. One minor improvement: getASTMembers could break out of the loop if a fatal error got created. In this case the refactoring will not work.
Released patch as well.
Created attachment 6016 [details] Fix 4 > getASTMembers could break out of the loop if a fatal error got created. I didn't do it because I wanted to show all errors if there are multiple. But it's useless, since RefactoringErrorDialogUtil doesn't show the JavaStatusContexts and therefore all messages look the same.
Released patch and marked PR as fixed.
start verifiying...
Verified using I20031007 + plugin-export for I20031008
