| Author |
 Topic  |
|
|
RobG
New Member
USA
3 Posts |
 Posted - May 04 2007 : 2:50:40 PM
|
Just a few suggestions that would improve the usability of the "extract method" refactoring tool (VC6.0 and others)
1. Make it a user settable option to always place implementation of new method in source file. It is rather rare that I want the new method as inline code in the .h file.
2. Instead of opening the simple "new method name" dialog that you do now, open the "change signature" dialog (or one similar to it). Extract method usually creates far more parameters than needed, and it would save a step if the signature could be changed here as well as the new name.
3. Make it a user settable option to specify the default visibility (public,private, protected) of methods produced with "extract method",
the current default of public is rarely the right answer for me.
4. Provide a chance to specify the visibility on the new dialog (2 above). |
|
|
feline
Whole Tomato Software
United Kingdom
19022 Posts |
Posted - May 04 2007 : 3:44:18 PM
|
We are considering some changes to Extract Method to let you change the signature of the new function:
case=3462
Do you have a simple example of Extract Method producing to many parameters? It is only supposed to produce the required parameters. If you were able to change the number of parameters being passed at this stage this would break Extract Method since VA would not know what to do with the variables. |
zen is the art of being at one with the two'ness |
 |
|
|
mmb
Senior Member
42 Posts |
Posted - May 04 2007 : 5:48:04 PM
|
While I'm not sure what these "to many parameters" are, I have a suspicion:
Example
int bla(int x, int y) {
int t;
t = x;
x = y;
y = t;
}
Now, selecting the 3 swap lines and doing an extract method produces:
static void MyMethod(int &t, int &x, int &y) {
t = x;
x = y;
y = t;
}
As the declaration of t was not selected, t is a parameter, but normally not wanted :-).
To detect that t is no more used in bla() a live analysis would be needed (and you don't run it in VA, right?).
However, if the user remove the int &t from the modified "new method name" dialog, VA could simply place a declaration of the right type at the beginning of the newly created method. This would produce a correct function, ie. if "int &t" is deleted from the parameter list, create
static void MyMethod(int &x, int &y) {
int t;
t = x;
x = y;
y = t;
}
This should be easy to implement and may produce the desired result.
|
|
|
|
RobG
New Member
USA
3 Posts |
Posted - May 05 2007 : 09:36:07 AM
|
The reply by mmb illustrates the situation reasonably well, but represents a very simple case. This usually happens when refactoring a large smelly method by extracting sections of the code into new methods. those extractions don't include a few of the local variables (used throughout the original method), but the intent is to redeclare them as locals in the new extracted methods. As it stands now, I accomplish this manually, by using change signature after the extract method, adding the new declarations by hand. The approach suggested by mmb goes a bit further toward automating things, and would be a welcome solution.
Any comment on the implementation location and visibility suggestions?
|
|
|
|
accord
Whole Tomato Software
United Kingdom
3287 Posts |
Posted - May 06 2007 : 10:14:55 AM
|
"It is rather rare that I want the new method as inline code in the .h file."
Yes  After extract method, I almost always have to use "move implementation to source" feature.
Please, consider to add a checkbox "create in source" or something. And it should be checked by default 
And a checkbox to insert the new function after/before original would be also great
Now, my extract method steps usually:
1. Extract method
2. Move to implementation
3. Move implementation after/before original function by VA outline or by hand (cut and paste).
(If I have a lot of functions, it takes time to find the original function in VA outline.)
What do you think? |
Edited by - accord on May 06 2007 10:33:42 AM |
|
|
|
accord
Whole Tomato Software
United Kingdom
3287 Posts |
Posted - May 06 2007 : 10:49:55 AM
|
mmb has described a real problem.
I'am using a workaround to these situations:
In mmb's situation I select "int t" also.
When variables are spread across the function, I'm collect them before the block I will extract (cut and paste). If other uses are presents, I keep a copy at the original location also (a great help the red ~~~~ when moving variables)
Even better if visual assist can do it automatically, but it will introduce an another problem: it will produces some unused variables in original code. Ideally it should also be deleted by VAX, or can be deleted by hand, following the compiler warnings (unreferenced local variable) |
|
|
|
feline
Whole Tomato Software
United Kingdom
19022 Posts |
Posted - May 07 2007 : 07:27:43 AM
|
First the question of placement of the extracted function - header or cpp file. We are discussing this internally at the moment, trying to find a good general solution. The current refactoring operations are designed to be small and simple, so there is no need for options, extra questions, or a 16 step wizard There is a certain logic to this design decision, but we could do with some way of saying "do this then do this".
Currently you can use IDE macro's to chain together two or more VA refactoring operations, but I am aware that this is not as simple and easy as it could be:
http://docs.wholetomato.com?W382
Where the new function appears, above or below, we are also discussing an improvement to this:
case=6374
The local variables, this is a whole new question. Currently we have existing bug reports that show the opposite effect. Consider the following function:
static void testExtractLocalVariable()
{
int nFoo; // select this line
nFoo = 1; // and this line
nFoo++;
}
and now select the two marked lines only and trigger Extract Method. This is over simplified, deliberately, but it shows the problem. When the variable declaration is selected Extract Method is designed to move the variable to the new function, which is what people are saying they want to happen.
My concern for now is that if we try and extract code that is not selected we are likely to run into unexpected edge cases, or people are going to get upset since VA is not doing what they asked. I may be being overly cautious here, but the open bugs for Extract Method show that "if it can go wrong, it will", since people will select and extract quite unexpected pieces of code.
For now getting some basic sanity checks up and running, to warn you when Extract Method has been asked to do something that is not going to work seems the best approach:
case=4510
Having said that I have put in a feature request for Extract Method to check and work out if all uses of a local variable have been selected, so it can extract the declaration as well, since this is an interesting idea and I don't want us to forget about it:
case=6421 |
zen is the art of being at one with the two'ness |
|
|
| |
Topic |
|
|
|
Suggestions to improve extract method refactoring
