Community
Participate
Working Groups
Build Identifier: M20110909-1335 We have recently conducted an empirical study on the use of Eclipse refactoring tool using our custom data collector called CodingSpectator <http://codingspectator.cs.illinois.edu/>. We studied the refactoring activities of 26 programmers for a total of about 1400 hours of programming. See our technical report <http://hdl.handle.net/2142/27730> for more details. One of the findings of our study was that our participants were not able to use the Move Instance Method refactoring tool. We investigated the invocations of this refactoring and asked some of our participants about their use of it. As a result, we found a mismatch between how users expected the Move Instance Method refactoring tool to work and the current design of the tool. In the following, I'll briefly describe the current current design of the Move Instance Method refactoring in Eclipse, explain the problems of our participants with this design, and finally propose a few alternatives to mitigate these problems. Reproducible: Always
Created attachment 207597 [details] A screenshot of the current design of the input dialog of the Move Instance Method refactoring tool To use the Move Instance Method refactoring tool, the user selects a method and invokes the refactoring. Then, the tool computes the possible targets of the move operation. For example, if the user invokes the Move Instance Method refactoring on method C.m of the following piece of code, the refactoring tool will open the dialog shown below. Note that the possible destinations of the move operation are variables not classes. ---- //C.java public class C { D d1; D d2; void m(E e1, E e2) { } public static void main(String[] args) { E e1 = new E(); E e2 = new E(); C c = new C(); c.m(e1, e2); } } class D { } class E { } ----
CodingSpectator reported that six of our participants attempted to apply the Move Instance Method refactoring tool for a total of 16 times. Surprisingly, all of them failed to perform the refactoring. On average, our participants spent 16.5 seconds configuring the Move Instance Method refactoring, which is significantly longer than what it takes to configure most automated refactorings, i.e. eight seconds. Our participants received the following error message from the Move Instance Method refactoring five times: > This method cannot be moved, since no possible targets have been found. > > Only a class which is reachable from within this method can be a valid target. The target must therefore be the declaring class of a parameter or field type. In addition the target must be writable. Out of the 16 failed attempts to apply the Move Instance Method refactoring, at least six of the failures could have been easily avoided had the tool inferred that the selected instance method could safely be made static. CodingSpectator reported that one of our participants had spent about half a minute on the input dialog of Move Instance Method refactoring. We expected the configuration of this refactoring to take less than ten seconds. So, we asked this experienced participant why he had spent so much time on configuring this refactoring. The participant told us that he had been confused by the way the tool had presented the possible destinations of the move refactoring. The participant was confused that the refactoring tool listed the variables that were possible targets of the move. So, he decided to cancel the refactoring. The participant expected to see just a list of class names to which the method could have been moved. This is the main mismatch between the expectations of the users of the refactoring tool and the actual design of the tool. The Move Instance Method refactoring tool asks the user to select a variable as the target so that the tool is able to to update the call sites of the selected method with the new receive. But, users just expect to have to provide the destination class. As a result, the Move Instance Method refactoring does not work when the users invoke it or cannot move the selected method to a class that the user has in mind. This mismatch accounts for most of the failures of our participants in applying the Move Instance Method refactoring tool.
Created attachment 207599 [details] The first alternative of the input dialog of the Move Instance Method refactoring makes a small change to the current design of the input dialog of the Move Instance Method refacotring. The user expects the refactoring tool to ask for a destination class. So, we suggest alternative designs of the refactoring tool to match this user expectation better. For example, one way to make the required input more clear is to break the destination variable into two parts: "destination class" and "new receiver". See the attached figure. First, the user decides which class to move the method to by selecting the destination class from the first list. Having selected the destination class, the refactoring dialog populates the second list by all the variables of the selected type that could become the new receiver of the new method. Finally, if there are several candidates for the new receiver, the user will select one from the list.
Created attachment 207601 [details] In the second alternative design of the input dialog of the Move Instance Method refactoring tool, the tool may infer that the selected method can be safely made static. Another way of making the refactoring tool more flexible is to detect if the selected method can be safely converted to a static method. If the selected method doesn't depend on any instance fields or methods of the enclosing class, it could be made static. Once the method is made static, it can be easily moved to any class. In this case, the refactoring dialog would only need to ask for the destination class. See the attached figure for how the dialog could be simplified in this case.
Created attachment 207603 [details] The third alternative simplifies the design of the input dialog of the Move Instance Method refactoring by making the selection of the new receiver of the selected method optional. Another alternative is to make the refactoring even more flexible and make the selection of the new receiver optional. See the attached figure. If the user doesn't choose a new receiver, the refactoring tool will check whether the selected method could be safely converted to a static method or not. If the method could be made static, it will make it static and then move it to the destination class. Otherwise, the refactoring will convert the selected instance method into a static method of the destination class. To convert the instance method to a static method, the refactoring tool will add a parameter to the static method that plays the role of the "this" object in the original instance method and adjust the visibilities of the fields and methods of the enclosing class of the selected method so that they can be referenced in the new static method. In the proposed design, if the user selects method C.m() in the following code and decides to move it to class E, the refactoring tool will replace method C.m() by a static method E.m(C) as shown in the final snapshot of the code: Before: ---- //C.java public class C { D d; void m() { d.n(); } public static void main(String[] args) { new C().m(); } } //E.java public class E { } ---- After: ---- //C.java public class C { D d; void m() { d.n(); } public static void main(String[] args) { E.m(new C()); } } //E.java public class E { void m(C c) { c.m(); } } ----
In summary, our study has shown that the current design of the Move Instance Method refactoring tool is hard to use because it limits the move operation to reachable variables. The main problem is that the refactoring tool asks the user about the new receiver of the method while users think about the new enclosing class of the method. We have proposed three alternatives to make the tool more flexible and intuitive. There seems to be some overlap between the changes requested in this bug, Bug 118032, and Bug 10605. So, there is a potential for code reuse in the implementation of these change requests.
Thanks guys for sharing your findings and for all the detailed bug reports! :-) (In reply to comment #1) > Created attachment 207597 [details] > A screenshot of the current design of the input dialog of the Move Instance > Method refactoring tool Indeed, this dialog is not great. (In reply to comment #3) > The user expects the refactoring tool to ask for a destination class. So, we > suggest alternative designs of the refactoring tool to match this user > expectation better. For example, one way to make the required input more clear > is to break the destination variable into two parts: "destination class" and > "new receiver". This makes sense. However, the following should be simpler to implement while being equally effective - Move 'Type' to the left column and change the name of the variable column from 'Name' to 'New Receiver'. (In reply to comment #4) > Another way of making the refactoring tool more flexible is to detect if the > selected method can be safely converted to a static method. If the selected > method doesn't depend on any instance fields or methods of the enclosing class, > it could be made static. Once the method is made static, it can be easily moved > to any class. Now we also have the compiler diagnostic 'Method can be made static'. Maybe we can leverage the feature or its implementation here? Once this situation is detected we should really use the 'Move Static Members' refactoring. In any case, this goes a bit beyond improving the refactoring dialog and should be handled in a separate bug. (In reply to comment #5) > Another alternative is to make the refactoring even more flexible and make the > selection of the new receiver optional. This also goes a beyond improving the refactoring dialog and should be handled in a separate bug.
(In reply to comment #7) > However, the following should be simpler to implement while > being equally effective - Move 'Type' to the left column and change the name of > the variable column from 'Name' to 'New Receiver'. Fix for this will go in org.eclipse.jdt.internal.ui.refactoring.MoveInstanceMethodWizard.MoveInstanceMethodPage.createControl(Composite)
(In reply to comment #8) > (In reply to comment #7) > > However, the following should be simpler to implement while > > being equally effective - Move 'Type' to the left column and change the name of > > the variable column from 'Name' to 'New Receiver'. > Fix for this will go in > org.eclipse.jdt.internal.ui.refactoring.MoveInstanceMethodWizard.MoveInstanceMethodPage.createControl(Composite) I think it's a good first step to change the column header to "New Receiver". However, more work is required to enable programmers to use the Move Instance Method refactoring.
Created attachment 207774 [details] screenshot of new design (In reply to comment #8) > (In reply to comment #7) > > However, the following should be simpler to implement while > > being equally effective - Move 'Type' to the left column and change the name of > > the variable column from 'Name' to 'New Receiver'. > Fix for this will go in > org.eclipse.jdt.internal.ui.refactoring.MoveInstanceMethodWizard.MoveInstanceMethodPage.createControl(Composite) Fixed this part in master - a83a65bf46366a0d6ecdd9ac36da2c19a4e05268
I opened bug 365286 to discuss conversion to a static method. Marking this bug as FIXED.
