| Author |
 Topic  |
|
|
ckline
New Member
5 Posts |
 Posted - Feb 05 2014 : 09:23:51 AM
|
A refactoring option for "Extract variable" (a.k.a. Extract Constant, Extract Local, etc) is something many people have asked for.
If you search for "case=1492" you will see that it has been an active feature request since 2006, and continues to receive requests for implementation.
Our team is very much in need of this functionality. As Visual Assist users for the last 10+ years we would like to stick with Visual Assist, but are considering switching to CodeRush or another plugin for this functionality if it's not likely to become available soon for C++ and C#.
Is this being worked on currently, or planned for the near future?
Thank you.
|
|
|
accord
Whole Tomato Software
United Kingdom
3287 Posts |
Posted - Feb 05 2014 : 4:16:14 PM
|
Extract variable is a popular request, indeed. We have a lot of outstanding feature requests, and can't do everything at once 
I have increased the priority of this further, though:
case=1492
So hopefully we will get to this sooner. |
 |
|
|
sean
Whole Tomato Software
USA
2817 Posts |
Posted - Nov 07 2014 : 10:08:56 AM
|
| case=1492 is implemented in build 2052 |
|
|
|
ckline
New Member
5 Posts |
Posted - Nov 10 2014 : 1:22:56 PM
|
Thanks for listening, and for implementing this.
Unfortunately I've been having issues getting it to work as expected.
Issue 1:
I have been able to get it to work within a toy solution, though with strange inconsistencies. For example:
int boop()
{
return 2;
}
void bar()
{
int x = 5;
int y = x + boop();
int z = x + boop();
int a = x + y + z;
}
If I select the first occurrence of "x + boop()" and choose "introduce variable", I'm given the option of "replace occurrence" or "replace 2 occurrence(s)". However, if select the second occurrence of "x + boop", I'm not given the option of "replace 2 occurrence(s)" and only the selected occurrence is replaced.
Issue 2:
I have been unable to get it to work successfully at all, even in simple methods, within our own C++ codebase under VS2013. No matter how simple the expression selected, and even in functions with only a single scope (no conditionals), every time I attempt to introduce a variable I am given an error dialog that says "Introduce Variable is not supported at this position".
In fact, even when I paste the above code (which worked successfully in a toy solution) into a function in our own codebase, I receive the same error.
Can you please provide any guidance on the conditions under which this feature will work versus not work?
Thank you. |
|
|
|
feline
Whole Tomato Software
United Kingdom
19023 Posts |
Posted - Nov 10 2014 : 8:42:26 PM
|
For issue number 1, I am seeing the same effect here. Thank you for the clear description.
case=86227
This is a new refactoring command, and there tend to be the occasional rough edge at first, so it is good to get some feedback on this.
For issue 2, that is strange. At a guess, this sounds like something further up the file, or in one of your common header files confusing us. Can you please try adding your test code to a new, empty cpp file inside your main solution? A file that has no #include statements, nothing else, just the test code. This should help to tell us if the solution its self is a factor. |
zen is the art of being at one with the two'ness |
|
|
|
feline
Whole Tomato Software
United Kingdom
19023 Posts |
Posted - Nov 11 2014 : 7:37:18 PM
|
For issue number 1, I did not read the documentation carefully enough, this is actually by design:
https://wholetomato.fogbugz.com/default.asp?W514
quote:
Only occurrences in the current scope that follow the selection can be replaced. If you want to introduce a variable for every occurrence of an expression in a block, make sure you select its first occurrence.
so the bug report has become a feature request, since I agree that this is a little unexpected. It does make sense as a simplification for the first version of the command though. |
zen is the art of being at one with the two'ness |
|
|
|
ckline
New Member
5 Posts |
Posted - Nov 13 2014 : 10:21:08 AM
|
quote:
For issue 2, that is strange. At a guess, this sounds like something further up the file, or in one of your common header files confusing us. Can you please try adding your test code to a new, empty cpp file inside your main solution? A file that has no #include statements, nothing else, just the test code. This should help to tell us if the solution its self is a factor.
I am unable to reproduce this issue today. A few things have happened since the original bug report (upgraded from VS2013 Update 4 CTP to Update 4 RTM, clean build, etc), but I'm not sure what fixed it. Perhaps some data got regenerated, or it was just a temporary fluke. If it occurs again I'll try to narrow it down.
quote:
Only occurrences in the current scope that follow the selection can be replaced. If you want to introduce a variable for every occurrence of an expression in a block, make sure you select its first occurrence.
I see now. While I understand the behavior, that makes it cumbersome to use in practice.
For example, when trying to introduce a variable for a repeated statement within nested scopes:
// common case 1
if (value)
{
DoSomething(foo()->bar()->slowFunction());
}
else
{
DoSomethingElse(foo()->bar()->slowFunction());
}
// common case 2
switch(value)
{
case 1: foo()->bar()->baz()->DoSomething1(); break;
case 2: foo()->bar()->baz()->DoSomething2(); break;
case 3: foo()->bar()->baz()->DoSomething3(); break;
default: foo()->bar()->baz()->DoSomething4(); break;
}
In these very common cases you have to manually introduce a variable for foo()->bar()->baz() at each invocation point, when it would be preferable to have a way to introduce it outside the scopes in which the occurrences appear.
What would be most useful from my perspective would be to have the following options:
- replace selection (when only one occurrence, or the selected occurrence is not the first)
- replace N subsequent occurrences in current scope (replaces all occurrences in current scope that fall at or after the selected one)
- replace all occurrences in current scope (replaces all occurrences in current scope and places the introduced variable above the first occurrence)
- replace all occurrences in current function (replaces all occurrences found in all scopes in the innermost function containing the selected occurrence; introduced variable is placed in the innermost scope that encompasses all occurrences)
I would actually use the "all occurrences in current function" most of the time.
|
|
|
|
accord
Whole Tomato Software
United Kingdom
3287 Posts |
Posted - Nov 14 2014 : 6:08:32 PM
|
Thank you for the feedback, I have put in an enhancement request to search in outer scopes:
case=86322
However, I think it should be more automatic. I mean, in your example, all references starts with the same method (foo), so all instances can be replaced safely.
Regarding the problem: next time it happens, can you please try whether reparsing the file helps?
VASSISTX -> Tools -> Reparse current file
You shouldn't need to do this, I'm just wondering whether it is a refresh issue or something else. |
Edited by - accord on Nov 14 2014 6:55:35 PM |
|
|
|
ckline
New Member
5 Posts |
Posted - Nov 17 2014 : 10:03:13 AM
|
quote:
However, I think it should be more automatic. I mean, in your example, all references starts with the same method (foo), so all instances can be replaced safely.
I was thinking of a case (different from my example code) where those function calls had side-effects, so the person doing the refactoring might want more control over which references were replaced. But I agree, having be automatic and put the introduced variable in the outer scope would be what you want 99% of the time. If I had that, I could live without the additional control.
quote:
Regarding the problem: next time it happens, can you please try whether reparsing the file helps?
Yes, I'll give this a try if it crops up again.
|
|
|
|
accord
Whole Tomato Software
United Kingdom
3287 Posts |
Posted - Nov 18 2014 : 1:53:19 PM
|
Thank you for your observations they are helpful.
Since non-const methods, references in parameter lists, etc. can complicate matters. Maybe the most helpful would be for the next step (regarding this problem) to introduce one more menu item as you suggested?
Replace selection
Replace in scope
Replace all instances
Certainly, when there are only instances in the scope, we should call the 2nd item as "Replace all instances". But we should implement case 86227 before iterating toward this direction. |
Edited by - accord on Nov 18 2014 1:54:57 PM |
|
|
|
ckline
New Member
5 Posts |
Posted - Nov 18 2014 : 4:57:46 PM
|
quote:
Maybe the most helpful would be for the next step (regarding this problem) to introduce one more menu item as you suggested?
This sounds like a fine plan to me.
|
|
|
| |
Topic |
|
Refactor: Status of case 1492 "Extract Variable?"
