| T O P I C R E V I E W |
| jfreeman |
Posted - Jan 08 2007 : 3:08:18 PM
Extract Method doesn't work if the "break" statement crosses the function boundary after the refactor.
This:
void main()
{
int x = 1;
while(x++)
if (x > 3) break;
}
Incorrectly refactors to:
void MyMethod( int x )
{
if (x > 3) break;
}
void main()
{
int x = 1;
while(x++)
MyMethod(x);
}
VA_X.dll file version 10.3.1543.0 built 2006.12.19
VS 2005 SP1
|
| 12 L A T E S T R E P L I E S (Newest First) |
| feline |
Posted - Jan 15 2007 : 08:42:33 AM
The conditional return statement actually uses the test code:
static bool bugExtractConditionalReturn()
{
// extract method - start here
if(1 > 2)
return true;
// extract method - end here
return false;
};
If you run the test you will see why this is definitely a bug  At the same time, if you really trigger Extract Method in this case then there is not a lot we can do to help. The only sensible solution I see is a warning message.
I can see it now, Extract Method, "helping you make mistakes faster"  Certainly the programmer can make a mistake without VA's help, and the current Extract Method does allow you to change how the code functions, and create all sorts of strange side effects if you try.
We try to think of all the edge cases first, and protect against them, but there are so many edge's that it is common to miss a few, despite our best efforts.
As for "please use the recommended coding style", for now I think this FAQ covers it well enough 
http://docs.wholetomato.com?W147 |
| sl@sh |
Posted - Jan 15 2007 : 03:28:11 AM
Like feline said: in an ideal world any function would only have one exit point at the end (and by the way, feline: even in long functions I always try to accomplish that standard, mostly by introducing some kind of 'finished' flag that I keep inquiring about again and again. Not always a good solution but at least you always know exactly that parts of the code might not be executed under certain conditions - which is not the case when you insert return statements at random)
That said: it's quite possible that code with return statements inside (and not at the end) is in fact semantically correct, even though it might not be good coding style[TM]. So while Extract Method might check for internal return statements, the only thing that should cause it to stop and inquire would be if those return statements return a value that doesn't match the type of any suitable variable or result Extract can find for the method end return value. (and at the moment I can't even think of an example for such a case - not sure if that is actually possible with code that does compile)
Of course, just to teach the programmers a lesson on 'good coding style'[TM] Extract Method could decline code containing internal return statements, but I doubt that would be a good policy, especially since many of those would likely beg to disagree on what 'good coding style'[TM] actually is  |
| accord |
Posted - Jan 13 2007 : 09:20:29 AM
Great, but "selecting half a variable name" and "selecting mismatched brackets" will lead to compiler error. Alt+Backspace (undo) and you can try again.
But if there is 1 return in a longer selection (in a function that returns void), a working code will change it's functionality without any compiler error. It is dangerous itn't it? And it may be easy to check this case.
A "powerful" programmer also can make mistakes, or not? 
What do you think? |
| feline |
Posted - Jan 12 2007 : 10:37:46 AM
I have already put in a request for Extract Method to check for a conditional return statement inside the extracted code.
case=4126
sl@sh in an ideal world all functions would only have one exit point, at the very end I think it is safe to say most of us do not program in this ideal world. Plus there are times when this just does not make much sense, at least for longer functions.
Currently Extract Method just does exactly what you ask, on the theory that you, being the all powerful programmer know what you are doing. Over time I can see that we are going to need to add more checks to Extract Method, but currently this is a fairly low priority since it is not so much a bug as an unexpected "bonus feature"
Still I have started a collection of basic checks we could perform.
* selecting half a variable name, sl@sh I had not even thought to try that
* selecting mismatched brackets
case=4510 |
| accord |
Posted - Jan 11 2007 : 05:19:35 AM
Ok, sorry, my description wasn't clear.
I mean "return;" keyword in the middle of a void funcion. |
| sl@sh |
Posted - Jan 11 2007 : 04:01:07 AM
Why should there be a return statement? For one, void functions don't need any. Secondly, many functions that are not void only provide one return statement at the end. If I want to extract code from such a function, chances are that the code to be extracted doesn't contain a return statement either.
I don't think there's *anything* extract method should sensibly expect within the selected code. The only thing it might want to check is that the selection doesn't start or end in the middle of a symbol. (I just tried, it's not checked!)
|
| accord |
Posted - Jan 10 2007 : 4:21:25 PM
Firstly it can be easy for VAX to spot "return" statements in selected code (or inside a define) when I do an extract method.
If I do not spot a return statement in a larger selection it can break a working code without a compiler error, which is dangerous.
|
| feline |
Posted - Jan 09 2007 : 3:11:34 PM
refactoring in VA is a fairly new feature, it was only introduced with the 15xx builds, and we are still working some of the kinks out of it
On paper spotting these sort of errors is sensible, but it is not always easy, due to the things people do with C / C++, especially anything involving #define's. As I understand it Java, like C#, is a much easier language to parse, and does not have some of the more "interesting" features of C++.
I am just wondering if I could even draw up a list of all error cases that would need to be checked, and I am not sure how easily I could do so. |
| jfreeman |
Posted - Jan 09 2007 : 1:26:06 PM
Right, I used Extract Method on the single line. I'd expect VA to flag this as an error (maybe "continue" or "goto" cause similar problems?). I'd expect if someone selects an opening bracket but not the matching closing bracket VA would flag this as an error as well.
I'm used to a Java IDE where Extract Method would catch these type of errors, so I was assuming VA would also. Maybe by design VA is not intended to operate at that level?
|
| feline |
Posted - Jan 09 2007 : 07:50:39 AM
Did you just use Extract Method on the single line:
if (x > 3) break;
If so, then I am not sure what VA could do about this. Strictly speaking Extract Method has worked perfectly, since it has extracted the code you asked it to extract. Unfortunately doing this has significantly changed the meaning of the code.
Asking VA to detect this is one answer. However when you stop and ask "what do we have to detect" I am not sure how big an issue this could become. What if someone selects an opening bracket but not the matching closing bracket? |
| jpizzi |
Posted - Jan 09 2007 : 12:39:46 AM
When posting code, select the button with the '#' character on it. This will provide you with the tags to surround your code so that the forum doesn't reformat the lines as if it were standard html text. |
| jfreeman |
Posted - Jan 08 2007 : 4:15:15 PM
sorry the indents (space chars) are gone -- how to I prevent that when I post? |
