| T O P I C R E V I E W |
| legalize |
Posted - Jan 22 2014 : 10:28:28 PM
Say I have a ridiculously long method that returns a bool:
bool A::crazy_long_method() {
if (!check_condition()) {
return false;
}
if (check_other_condition()) {
return false;
}
// lots more blocks of code like the above
return true;
}The intention of this method is that there are a large number of steps that must be successfully performed in order for this method to return true. Along the way, there may be steps that detect errors and shortcut return with a false value.
When we apply Compose Method to this, we want to extract a series of steps so we can end up at:
bool A::crazy_long_method() {
return step_one()
&& step_two()
&& step_three()
&& step_four()
&& step_five();
}
bool A::step_one() {
if (!check_condition()) {
return false;
}
return true;
}
We can get there by typing in a tiny bit of code before we Extract Method to help VAX get the signature right and complete the method body. If we try to extract this chunk:
if (!check_condition()) {
return false;
}
Then we have left it ambiguous what the return value should be on the extracted method when step_one() returns true. Fix this by inserting the necessary return value before extracting:
if (!check_condition()) {
return false;
}
return true;
Then we get a nice method extracted and crazy_long_method looks like this:
bool A::crazy_long_method() {
return step_one();
if (check_other_condition()) {
return false;
}
// lots more blocks of code like the above
return true;
}Now we can work on the next code block to get:
bool A::crazy_long_method() {
return step_one();
return step_two();
// lots more blocks of code like the above
return true;
}Which we can fix up with a small amount of hand editing to get:
bool A::crazy_long_method() {
return step_one();
&& step_two();
// lots more blocks of code like the above
return true;
}Once we are done, we'll have composed crazy_long_method into a series of calls to other methods, all at the same level of meaning. If we need to know the details of any particular step, we can drill down, but now the overall behavior of crazy_long_method is visible at a glance.
http://www.industriallogic.com/xp/refactoring/composeMethod.html |
| 7 L A T E S T R E P L I E S (Newest First) |
| sean |
Posted - Nov 07 2014 : 10:08:52 AM
case=1492 is implemented in build 2052 |
| legalize |
Posted - Jan 28 2014 : 2:00:20 PM
Introduce Local Variable and Inline Local Variable are duals of each other, obviously. Having both as automated refactorings is what lets you do these larger transformations in a completely automated way. If I only had one and not the other, then we're back to hand-edits which is where all my bugs come from when I restructure code. :) |
| accord |
Posted - Jan 28 2014 : 12:13:36 PM
Thank you for the detailed description. Considering this explanation, I think that Introduce Local Variable is more like
case=1492
I'm looking forward to use this myself, and can even recall the case number. Not a hard one to remember though  (http://www.eyewitnesstohistory.com/columbus.htm)
We are also considering inline local variable:
case=31485
As well as implementing something like your mentioned "Move Method":
case=70964 |
| legalize |
Posted - Jan 26 2014 : 1:22:07 PM
For Compose Method, Introduce Local Variable, Inline Local Variable and Split Local Variable are the two that you really need to break up a long method without making manual edits.
Split Local Variable is what you need when a local variable is assigned, used, and then re-assigned and used again. In order to break up the two chunks of code into distinct methods you need to disconnect their two independent uses of the same variable.
Introduce Local Variable is what you need to extract duplication automatically. Otherwise VAX will Extract Method with the wrong signature and you'll have to change it either manually (error-prone) or with Change Signature which has a tendency to leave incorrect call points in place. When you have the same structure of calls, but with different sets of arguments that are member variables, VAX will omit those variables as arguments in the extracted method because it detects that they are member variables. However, in order to eliminate the duplication, you need to the extracted methods to accept the member variables. In order to trick VAX into doing what you want, you temporarily Introduce Local for the arguments in the block of code you wish to extract, Extract Method to get the desired signature and then Inline Local in order to get rid of the local variables you introduced temporarily. This sequence is also very common when ultimately you wish to Move Method on the newly extracted methods to move them to another class where they are more appropriate. In that case, you absolutely must pass in the member variables as arguments to the extract method because the other class won't have access to those member variables.
VAX's lack of Introduce Local Variable, Inline Local Variable, Split Local Variable and Move Method are serious stumbling blocks in achieving these sorts of refactorings in an automatic way for C++. I can do this with ease for Java code using IntelliJ. Being able to achieve these refactorings automatically without manual edits gives high confidence in these sorts of transformations. Forcing them to be done with manual edits introduces the risk of errors, even with peer review. Many times the kind of code you want to clean up in this regard lacks any sort of automated unit testing or acceptance testing, increasing the risk of errors introduced through manual edits by an order of magnitude. Due to the risk involved, many engineers simply will not attempt these sorts of transformations, leaving code that suffers from poor structure and design, i.e. not following SOLID principles for OO design.
https://en.wikipedia.org/wiki/SOLID_%28object-oriented_design%29 |
| feline |
Posted - Jan 25 2014 : 4:49:39 PM
VA's refactoring tools are designed as specific, well defined commands, that you can combine together, in the manor you need, to help you refactor your code, and this seems to be helping here, which is good to know
Your experience cleaning up code is valuable, especially if it gives you a good sense of what will help.
We are looking to add a refactoring command to split a local variable:
case=30487
We are also looking at a couple of different ways of creating new local variables, based on existing code. I suspect a good idea here would be to create a new local variable from an existing statement, e.g. making a new local variable and setting it to the return value of a function you have called:
case=67761 |
| legalize |
Posted - Jan 24 2014 : 11:07:56 PM
The best refactoring tools are those that easily allow me to compose larger transformations through a series of mostly automated smaller transformations. I recommend that the VAX developers go out and find the most disgusting C/C++ code that they can find -- lots of global variables, lots of really long methods/functions, lots of poorly chosen names -- and practice transforming it into reasonable code -- encapsulated global state, small methods/functions, reasonable names. That's when you really appreciate some refactorings that we don't have in VAX:- Split Local Variable
- Introduce Local Variable
...and probably some others I'm forgetting right now.
Refactoring is a skill that you can build up with practice. As you become more experienced with smaller refactorings you become more adept at making larger transformations safely. With a little more support in VAX, I could make those larger transformations with a minimum of hand edits and it's always the hand edits that end up having the mistakes. |
| feline |
Posted - Jan 24 2014 : 8:17:18 PM
Thank you for the interesting post, it is good to see you making such successful use of Extract Method |
|
|
