Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
Implementing extract method #3358
This is going to be first iteration of restoring
Because there are numerous cases to test, we want to get start with most simplest, dumbest implementation.
Basically, the first iteration should aim to:
Note that with 1b, we will disallow selections where it includes flow of control statements that can go to some places not necessarily within the user's selection. That possibly could also include error handling, and of course any
Maybe the initial iteration can indicate why it can't extract method (e.g.
I'm not sure there's such a thing as as dumb implementation of this refactoring, but I agree it's a very complex one to tackle, with a ton of traps along the way. An iterative implementation seems like a very good idea.
The original implementation was rather naive, but did a few things pretty well.
The refactoring works off the user's selection in the active code pane... like all refactorings, really. The difference is that here we aren't working with a selected
All refactorings need an
While it definitely sucks that a disabled command doesn't explain why it's disabled, I think consistency matters, and if all commands are contextually disabled, then Extract Method should be no different; IMO it's the wiki's job (or the website's? the RD News blog's? Stack Overflow?) to explain how to use a given command and why it's disabled under such or such condition.
So when should the refactoring be enabled?
I think that covers it.
Variables (and constants) in scope
The refactoring needs to know about what's in-scope for the selection:
So the refactoring looks at what's in-scope, and before it can pop a preview box and prompt for a name, it needs to:
As the user specifies a name for the extracted method, and picks/confirms inputs and outputs, the preview box updates accordingly.
Okaying the preview actually creates the previewed member immediately underneath the calling procedure it was extracted from, and replaces all lines in the selection with a call to that extracted method. For a function, the selected return value determines which local variable receives the returned value. A re-parse is then requested on the parser state.
The extracted method is always
Oh, also - the refactoring needs to be able to work off a
Edge case for
If the Exit Sub is in a conditional, we would also exclude the conditional block, so long as the Exit is the only statement in the block.
Thanks for the additional points; good to think about them.
Just to be clear, the first iteration will be dumb in the sense it won't try to refactor the variables from original procedure. It'll collect all identifiers and thus leave up to the user to decide whether it's a new local variable, a new input, or a new output.
Subsequent iterations will be a bit more smarter, automatically deleting variables from the original procedure that are now unused after extraction, auto-guessing which's output and which's parameters or handling edge cases of
Agreed, especially on the point that we must never write uncompilable code. That's just bad juju.
To be honest, the part about identifying code blocks is the hard part. We might even need to implement some caching somewhere (seems doing that in the
I wouldn't expand the user's selection: we should work with what the user is giving us.
If it's invalid, it's invalid.
x If foo < bar Then x 'do stuff x '.... End If
Should not be expanded to:
x If foo < bar Then x 'do stuff x '.... x End If
However convenient that might be. At least not in the first iteration, when other more important cases aren't handled.
In order to figure out code blocks, we'll need a parse tree listener (to pick up starting & ending line positions of all code blocks) that runs during one of the resolver passes (could be together with the "identifier references pass" I guess), and something to abstract the blocks; perhaps a
Actually, I wasn't talking about expanding selections vertically - only horizontally. Not going to try to add/remove extra lines; only to expand the start to the column 0 and the end to the column
I do think we have to implement a way to know when it's a statement for extract method to be meaningful. It simply won't any sense if we don't work at the statement level. That's also why I think selection has to be expanded at least to the statement. Otherwise, it'll be asking too much from the user to tell them that they must select everything from