Whole Tomato Software Forums
Whole Tomato Software Forums
Main Site | Profile | Register | Active Topics | Members | Search | FAQ
User name:
Password:
Save Password
Forgot your password?

 All Forums
 Visual Assist
 Feature Requests
 Refactor: Status of case 1492 "Extract Variable?"
 New Topic  Reply to Topic
 Printer Friendly
Author Previous Topic Topic Next Topic  

ckline
New Member

5 Posts

Posted - Feb 05 2014 :  09:23:51 AM  Show Profile  Reply with Quote
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
3164 Posts

Posted - Feb 05 2014 :  4:16:14 PM  Show Profile  Reply with Quote
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.
Go to Top of Page

sean
Whole Tomato Software

USA
2641 Posts

Posted - Nov 07 2014 :  10:08:56 AM  Show Profile  Reply with Quote
case=1492 is implemented in build 2052
Go to Top of Page

ckline
New Member

5 Posts

Posted - Nov 10 2014 :  1:22:56 PM  Show Profile  Reply with Quote
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.
Go to Top of Page

feline
Whole Tomato Software

United Kingdom
15984 Posts

Posted - Nov 10 2014 :  8:42:26 PM  Show Profile  Reply with Quote
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
Go to Top of Page

feline
Whole Tomato Software

United Kingdom
15984 Posts

Posted - Nov 11 2014 :  7:37:18 PM  Show Profile  Reply with Quote
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
Go to Top of Page

ckline
New Member

5 Posts

Posted - Nov 13 2014 :  10:21:08 AM  Show Profile  Reply with Quote
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.

Go to Top of Page

accord
Whole Tomato Software

United Kingdom
3164 Posts

Posted - Nov 14 2014 :  6:08:32 PM  Show Profile  Reply with Quote
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
Go to Top of Page

ckline
New Member

5 Posts

Posted - Nov 17 2014 :  10:03:13 AM  Show Profile  Reply with Quote
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.

Go to Top of Page

accord
Whole Tomato Software

United Kingdom
3164 Posts

Posted - Nov 18 2014 :  1:53:19 PM  Show Profile  Reply with Quote
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
Go to Top of Page

ckline
New Member

5 Posts

Posted - Nov 18 2014 :  4:57:46 PM  Show Profile  Reply with Quote
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.

Go to Top of Page
  Previous Topic Topic Next Topic  
 New Topic  Reply to Topic
 Printer Friendly
Jump To:
© 2019 Whole Tomato Software, LLC Go To Top Of Page
Snitz Forums 2000