| T O P I C R E V I E W |
| JohnCrenshaw |
Posted - Dec 13 2006 : 11:45:37 AM
I love the Extract Method feature. I also think that it could be greatly improved by a very simple change. I think that when you define the extracted function, there should be an option to select the parent class. For example, I frequently find myself writing code in some dialog and say to myself, "This doesn't belong here, it belongs as a separate function in such and such a class" currently, I have to mostly rewrite the code to work in the other class (by virtue of the fact that the this pointer has changed.) I can still extract the method (HUGE timesaver), but I have to move it to the class that it really belongs in and then fix up the pointers. I suspect that this pointer repair could be done easily within the rest of the extract method code.
To choose the scope really requires that the user identify the variable that becomes the this pointer. Just add a combo box for the this pointer, default value is 'this' (unless the scope is global) and all of the variables that have a writable class are listed (for example, a CString would have a class, but not an editable one, and int would not have a class, but a CMyClass object of sorts would provide a great this pointer.) |
| 14 L A T E S T R E P L I E S (Newest First) |
| feline |
Posted - Jan 08 2007 : 12:59:06 PM
Apologies for the delay in getting back to this.
I have put in a feature request for this, so we won't forget about it. I wouldn't hold your breath waiting for us to look into this  but it does represent an interesting idea.
case=4419 |
| JohnCrenshaw |
Posted - Jan 03 2007 : 11:43:36 AM
quote:
Originally posted by feline
quote:
Originally posted by JohnCrenshaw
At this point, if the user wants, a full move can be easily obtained by simply deleting the old function and having to do the same repairs to the code.
Ultimately, I suspect that this is the simplest implementation because the refactor code ALREADY has to deal with the variables.
John I hope you do not have a vision of VA coming along and fixing all of the calls as you change the signature of the calling function. This brings with it so many complications I don't even want to think about it.
No, I expect that changing the signature will require me do make a bunch of changes manually. I always make sure that any signature change I make will generate compiler errors with all existing references to make sure that I don't have any bugs sneak up on me.
quote:
which is why you keep talking about "this" pointers, since the variable "mc" becomes the class instance in the extracted code, correct?
Bingo. |
| JohnCrenshaw |
Posted - Jan 03 2007 : 11:35:18 AM
quote:
Originally posted by sl@sh
If you omit 'this' in the call, you cannot replace m_myothervar in the extracted method (of course, most likely m_myothervar is protected against access, so the reference has to be manually fixed anyway  ).
Ooooh! good point. Perhaps members would have to be passed through as parameters, rather than passing "this" from the current class. |
| sl@sh |
Posted - Dec 20 2006 : 06:57:42 AM
The call should include the CMyDialog 'this' pointer as a parameter as well, so should look like this:
mc.MyNewFunction(this, n, s, mc2);
If you omit 'this' in the call, you cannot replace m_myothervar in the extracted method (of course, most likely m_myothervar is protected against access, so the reference has to be manually fixed anyway ). |
| feline |
Posted - Dec 19 2006 : 5:39:24 PM
John there is no doubting the thought you have put into this. It is simply that sometimes it can be difficult to convey a complex idea through English
sl@sh the same problem with class hierarchies occurred to me as well while reading this. My initial answer is to ignore this problem. I say this since people have already expressed an interest in "push up" and "pull down", to move a function into the base class or a child class. Assuming for the moment this has been done (quite a large assumption ) then it would make more sense to rely on this to move the function a second time to the correct place in the class hierarchy.
quote:
Originally posted by JohnCrenshaw
At this point, if the user wants, a full move can be easily obtained by simply deleting the old function and having to do the same repairs to the code.
Ultimately, I suspect that this is the simplest implementation because the refactor code ALREADY has to deal with the variables.
John I hope you do not have a vision of VA coming along and fixing all of the calls as you change the signature of the calling function. This brings with it so many complications I don't even want to think about it.
I can just about picture what you are wanting to happen here, so I am going to look at your example again and see if it makes more sense now:
void CMyDialog::SomeFunction()
{
CMyClass mc;
CMyClass mc2;
CString s;
int n;
// Begin code to refactor
n++;
mc.DoSomething(s[n]);
mc2.DoSomethingElse(mc, n);
mc.myvar.AddSomething(s);
m_myothervar.Foo(mc2);
// end code to refactor
}
let us assume you have selected "mc" as the class instance to own the new function, you are going to end up with the function call:
void CMyDialog::SomeFunction()
{
CMyClass mc;
CMyClass mc2;
CString s;
int n;
mc.MyNewFunction(n, s, mc2);
}
which is why you keep talking about "this" pointers, since the variable "mc" becomes the class instance in the extracted code, correct? |
| sl@sh |
Posted - Dec 19 2006 : 03:57:54 AM
Your suggestion makes a lot of sense for any function that is fully implemented. But since VA has to deal with all kind of states the code is in, including incomplete implementations, I fear that selecting the class only from types used within the function may not be sufficient. Moreover, even if you think a function should be moved to a particular class, this class could also be a base class of one of the types used within the implementation. Actually this is something I do a lot more often than moving functions from one class to another that is not related at all. So in any case, to be useful, this feature should also allow to pick a class from the list of base classes from those classes used in the implementation, or base classes of the class the function is currently in. |
| JohnCrenshaw |
Posted - Dec 18 2006 : 2:47:24 PM
quote:
Originally posted by feline
For some reason I still have no idea what John is asking for, but sl@sh your description makes sense
...
Updating all calls to the function, I think this would have to be left to the programmer to fix manually, the same way Change Signature currently works.
We would need a GUI for selecting the destination class... This could look and function in a similar manor to FSIW...
As hard to understand as my explanation may be, I actually thought this through very carefully before submitting it. Fundamentally, to properly switch the class that a function belongs to REQUIRES that the appropriate this pointer conversions be made. To make that conversion requires that the user define which, if any, of the variables used in the code becomes the new 'this' (therefore making it's class the parent class.) If the code is given a global scope, it could be a static member of any class, but the conversion is always the same in that case. Ultimately, this is best done through a refactor, always.
Take the following example:
Suppose I want to move a function to another class. If I move it, I break all the places that I was calling it and have to update those before I can compile and run. If I instead use extract method on the whole body of the function, specify the appropriate variable as the new "this" (thereby making it's class the new parent class) what do I have? The old function that I wanted to "move" to a new class, actually becomes an encapsulating function for the call, the appropriate class now contains the code, and the program runs perfectly without any further changes. If you completely change the signature of the function to be in a new class and such, all places in the code that called it now have to be fixed.
At this point, if the user wants, a full move can be easily obtained by simply deleting the old function and having to do the same repairs to the code.
Ultimately, I suspect that this is the simplest implementation because the refactor code ALREADY has to deal with the variables.
As for the question of how to define the destination class, it is first important to realize that unless you want it to be a static member, there is no point in moving the code to a class that is not already used by some variable in the code snippet in question. In my code in the original post, it makes no sense to extract the method to a CSomeRandomClass unless it just becomes a static member. The code as a static member of any class will always be the same as the code for global scope.
I fear that I may have complicated this once again in all minds but my own so I will sumarize.
The method of selecting the appropriate class can be implemented as follows:
Provide a drop list with the following entries:
this(default)
global
static
<one entry for each variable in the code that instantiates a modifiable class>
provide an edit field for the class, if the scope is static, let the user type in a class, otherwise it is disabled, but shows the class that will recieve the function (as determined by the variable that takes the place of 'this')
The user uses the drop list to select the variable for the this pointer in the final function. Remember, it is not important to be able to select from a list of all the classes in the program. The only ones that make sense to use, are the ones that are used for variables in the code, and since a class may be used more than once, it is important to know which VARIABLE, not just which CLASS.
I hope this makes as much sense to others as it does to me.
|
| feline |
Posted - Dec 14 2006 : 09:19:05 AM
*ah* now I understand what is going on here. For some reason I still have no idea what John is asking for, but sl@sh your description makes sense
There is no point in focussing to much on step 3 for now, since we do not currently have step 2. Simply moving a function from class A to class B is going to be a fairly big step. I have just triggered Extract Method on some code that uses the "this" pointer explicitly, and VA understands that there is no need to pass "this" as a parameter.
So even if we ignore member variables we have to think about the "this" pointer, and converting it into a parameter.
Updating all calls to the function, I think this would have to be left to the programmer to fix manually, the same way Change Signature currently works.
We would need a GUI for selecting the destination class... This could look and function in a similar manor to FSIW...
I am just wondering what other edge case problems there are that need to be considered just to produce a reasonable feature request, let alone write the code. |
| sl@sh |
Posted - Dec 14 2006 : 03:47:50 AM
Finally understanding what John is referring to, I'd say this is something to implement as a seperate function. The refactoring then would consist of three steps:
1. extract the method with the existing method (VAX refactoring)
2. move the function to the right place (into the proper class) if it isn't already there
3. refactor the use of a particular variable that is of type pointer to class (where 'class' is the class this method now is a member of) by replacing it with a 'this' pointer.
The refactoring described in step three is the one John is missing.
P.S.: I'd like to add some kind of reverse functionality - when I'm moving a function away from a class I will obviously need to replace any uses of the 'this' pointer with something else! How about a refactoring method to that end? |
| JohnCrenshaw |
Posted - Dec 13 2006 : 2:32:32 PM
I posted one reply for each half of the above post that needed a reply, pleast read the one before this of you will have a hard time with this one.
quote:
This immediately points to one of the "fun" issues here, letting the programmer specify the target class.
this is acctually very easy. The target isn't a class, it is one of the variables already in use, or can be the current this pointer, OR can have a global scope.
|
| JohnCrenshaw |
Posted - Dec 13 2006 : 2:29:00 PM
quote:
Are you talking about "what class do I move the function into?" If so I do not understand why you are looking at the functions that are used.
Yes and no, if you notice, in my example I used two different variables of the same class, if you just know which class to place the function in, you don't know enough. To start with, the function that immediately replaces the code could be either mc.MyMethod(...) OR mc2.MyMethod(...). Furthermore, which of the two is used changes the code internally because in the one case mc is the this pointer in the call to the new method and in the other case mc2 is the this pointer. It isn't terribly HARD, just time consuming to do by hand.
I know this is an odd sounding feature request, but the fact of the matter is, I am more likely to want to extract a method because the code belongs in a DIFFERENT class, than I am because I just want to use it elsewhere in the same class. I frequently see code get littered with unencapsulated bits and pieces that really belong somewhere else. Extract Method is about having clean code but there is a huge part of that need that it just barely misses.
|
| feline |
Posted - Dec 13 2006 : 1:00:40 PM
Are you talking about "what class do I move the function into?" If so I do not understand why you are looking at the functions that are used. As the programmer I may need to move this function into CFoo, CBar, CChocolate, or any other class in my project. This immediately points to one of the "fun" issues here, letting the programmer specify the target class. |
| JohnCrenshaw |
Posted - Dec 13 2006 : 12:32:09 PM
Sorry, let me explain. The question is really, "Which variable becomes the this pointer?"
consider the following (simplistic) example
void CMyDialog::SomeFunction()
{
CMyClass mc;
CMyClass mc2;
CString s;
int n;
// Begin code to refactor
n++;
mc.DoSomething(s[n]);
mc2.DoSomethingElse(mc, n);
mc.myvar.AddSomething(s);
m_myothervar.Foo(mc2);
// end code to refactor
}
In the above example, under the current design, the this pointer remains the same (pointing to the CMyDialog class). There are at least 4 other valid this pointers however. This is best understood as a function of what code is given in place of the extracted method.
Currently:
MyMethod(mc, mc2, s, n);
and the function is declared as
void MyMethod(CMyClass& mc, CMyClass& mc2, CString& s, int& n)
However, this could also be replaced by any of the following so long as the class it was placed in was correct and the appropriate adjustments were made to variable usage in the new function.
mc.MyMethod(this, mc2, s, n);
mc.myvar.MyMethod(this, mc, mc2, s, n);
mc2.MyMethod(this, mc, s, n);
::MyMethod(this, mc, mc2, s, n);
In the above examples, the this pointer comes from the following sources, respectively:
mc
mc.myvar
mc2
global(no this pointer)
note that since CString is not modifiable and n is not an instance of a class, they can't become the this pointer for the new function.
Of course this changes the code because the this pointer in the new function is different than it was when the code was written. Here is the code for one of the 4 possibilities:
mc.MyMethod(this, mc2, s, n):
void CMyClass::MyMethod(CMyDialog* pMyDialog, CMyClass& mc2, CString& s, int& n)
{
n++;
DoSomething(s[n]);
mc2.DoSomethingElse(*this, n);
myvar.AddSomething(s);
pMyDialog->m_myothervar.Foo(mc2);
}
Hopefully this gives kind of an idea of what I am actually talking about.
|
| feline |
Posted - Dec 13 2006 : 12:08:51 PM
I understand the desire to move a function to a different class, but what do you mean about selecting the "this" pointer? The "this" pointer has to be "this" by definition. |
|
|
