"Move Members" refactoring

[retailcoder]

We need a refactoring that makes it easy to move a procedure from one module to another (possibly a new one).

For example, in a project where public procedures are systematically qualified with a module:

Private Sub Workbook_Open()
    'this handler will not run if macros are disabled, leaving the warning visible and the buttons invisible
    Macros.ToggleMacros True
    Macros.SetOrderDate Now
End Sub

Manually moving, say, the SetOrderDate macro to a module other than Macros would break all call sites.

We need a refactoring that allows selecting a member and move it to another module, and automatically update the call site qualifiers. For example moving the SetOrderDate macro to a new OrderFormMacros module would automatically update this Workbook_Open handler as follows:

Private Sub Workbook_Open()
    'this handler will not run if macros are disabled, leaving the warning visible and the buttons invisible
    Macros.ToggleMacros True
    OrderFormMacros.SetOrderDate Now
End Sub

Moving one member is great, but the refactoring should also provide a way to select members from a module, and move all selected members in a single operation.

[Vogel612]

FWIW that seems like a candidate for two stages of implementation...

1. Move a single member
2. Move selected members

[BZngr]

Prior to initiating a WIP PR, I wanted to provide a description of what I have been developing to create a MoveMemberrefactoring. I am hoping for some feedback especially regarding the 'rules' that I have adopted thus far for the various move scenarios. I'm hoping that getting some feedback from the following 'code-less' descriptions will be more efficient than having anybody wade through both a description and a WIP PR to extract my thinking thus far. I currently have an implementation that supports nearly everything described below.

The MoveMemberrefactoring that would support moving a selected Method, variable, constant, UDT, or Enum(NonMethods) from a StandardModuleor ClassModuleto an existing or new StandardModule. If the move cannot be executed, the issue(s)/conflict(s) preventing the move are presented to the user. If the move can be executed, the user is allowed to preview the elements (NonMethods and Methods) that will be moved to the destination module.

BTW: I am a bit conflicted on the use of the term Member on the UI and in the code. It seemed necessary to me to distinguish declarations having a DeclarationTypeflag of Memberfrom the list of declarations you get when you call DeclarationFinder.Members(QMN). So, for the most part I have adopted the term 'Method' to identify DeclarationType.Member in the following descriptions and my code.

In General:

1. The refactoring supports selection of a single element at this point. That said, I'm writing the code to handle multiple selected elements - which I think is an important next step to making this feature far more useful. My hope is that implementing a multi-element-select version is largely a change to the UI.

2. The refactoring analyzes the call tree and NonMethods needed to support a requested move. Based on a set of applied conflict analysis predicates, it determines if the move is possible. In some cases, accessibility is modified on moved elements. This is done to avoid outcomes that do not compile. The user is presented with the modified accessibility in a preview, so he can cancel the move if this is not what was intended.

3. The set of conflict analysis predicates depend on the source module type. (again, I've only implemented standard module destinations at this point).

4. Though yet to be implemented, I intend to support moving UDTdeclarations and Enumdeclarations, Private or Public, but only if explicitly selected by the user. (in other words, they will not get 'dragged-along' if referenced by the move's call tree).

5. Moved methods are inserted at the end of the destination module.

6. Moved NonMethods are inserted prior to the first method (if one exists) of the destination module.

7. The preview provided to the user only shows what elements will be moved from the source module. It does not attempt to show the complete contents of the destination module following the move. I felt that showing all destination content would make the move results hard to find due to DeclarationSectionand CodeSectionseparation if the destination module was large (see G.5 and G.6 above).

The UI:

1. The user is presented with a dialog consisting of 3 dropdowns: SourceModule, Member to Move, Destination Module. Note: Since the first version of this refactoring allows selection of only a single element, dropdowns seemed appropriate. I am sure that a version that supports selecting multiple elements will require something more like the ExtractInterface UI.

2. If the user invokes the refactoring with a NonMethod or Method selected, then the dialog pre-sets the SourceModule dropdown and selected element dropdown with the values.

3. The SourceModule dropdown is populated with all modules in the project.

4. The DestinationModule dropdown is populated with all StandardModules in the project.

5. The DestinationModule is an editable dropdown to allow naming a new StandardModule to receive the moved elements.

6. The Member To Move dropdown is populated with all declarations of the SourceModule except parameters and local variables/constants.

7. At the bottom of the dialog is a textbox that presents conflict messages/explanations (for non-Executable moves) or a preview (a generous use of the term) of what will be moved if the request is executable.

Currently, I am applying following rules are applied when analyzing a requested move:

1. StandardModuleto StandardModule

   1. Moves referenced Private NonMethods, and Methods if referenced exclusively within the call tree of the selected Method to move. Each moved element retains its previous accessibility.
   2. Does not move Public NonMethods or Methods if referenced by the selected Method. Scope resolution is added to the moved content as needed to reference the source module Public Methods and NonMethods.
   3. If a Public or Private NonMethod is explicitly selected by the user, it is allowed to move. Private NonMethods will have their accessibility changed to Public is referenced by retained Methods in the source module.
   4. Moves both Property Methods of a set/get let/get pair when either is selected as the method to move.

2. ClassModuleto StandardModule

   1. Moves referenced Private NonMethods, and Methods if referenced exclusively within the call tree of the method selected to move.
   2. Does not move Lifecycle Handlers (Class_Initializeand Class_Terminate).
   3. Will not move a method if it references a class module Public NonMethod (except constants - explained below) or Method.
   4. Public or Private methods can be selected to move. Private methods will be made Public in the destination module if referenced by the source module.
   5. Moves both Property methods of a set/get let/get pair when either is selected as the method to move.
   6. If the moved code references source module Public constants, the constant(s) will be moved to the destination module and call sites (of the Public constant) updated. This operation makes an assumption (on behalf of the user), that he is intending to refactor a public constant from an instance based constant to a non-instance based constant...Admittedly, this may be doing too much.
   7. Will not allow moves of methods that raise Events - or methods that reference another method that raises an event.
   8. Will not allow moves of event handlers - or methods that reference an event handler.
   9. Will not allow moves of interface members.
   10. Will not allow moves of members that implement an interface - or methods that references member(s) that implement an interface.

3. UserFormto StandardModule- All the same are Class Module to Standard Module, except 2.2:

   1. Does not move Private members having identifierNames starting with UserForm_.
   2. Does not move controls.

[retailcoder]

@BZngr will read more attentively later, but shouldn't it be possible to move a member from one class to another?

[BZngr]

Yes...though I intentionally deferred attempting that capability. TBH I was a bit intimidated by the scenarios where I would need to insert instantiation code of a different class within the logic-flow at the call site of the original class/member (especially in the cases of a WithMemberAccessExprContext). So, I passed on that capability in order to make progress with the rest.

[Vogel612]

@BZngr I have updated your comment to improve how it scans when reading. Hope you're alright with that :)

[comintern]

You've obviously put more thought into this that I have, but I don't see any potential problems that you've overlooked except for use of private, module scoped variables on the source component. I'm assuming this would also disallow a move per 2.iii? Would it increase the complexity too much to allow moving a private backing variable for a property if it is only referenced in the [GLS]et members being moved? It seems like disallowing this might make it functionally difficult to relocate properties.

Also, just a quick comment about the use of "Member" - the other term we use in the UI is "Procedure", although this obviously doesn't capture UDTs and Enums. Given the choice between "Member" and "Method", I would personally go with "Member" though - a lot of the VBA books that I have use "Method" and "Property" as distinct from each other so it may be a little confusing.

[BZngr]

Thinking the Class to Class move question through a bit more. I think a reliable approach to that type of refactor might be something like the following:

Source ClassModule before the move. User wants to move MyX and MyY properties.

Private mXValue As Long
Private mYValue As Long

Property Let MyX(arg As Long)
    mXValue = arg
End Property

Property Get MyX() As Long
    MyX = mXValue
End Property

Property Let MyY(arg As Long)
    mYValue = arg
End Property

Property Get MyY() As Long
    MyY = mYValue
End Property

Public Function CalculateArea() As Long
   CalculateArea = mXValue * mYValue
End Function

After moving MyX and the MyY properties. (currently, two operations) the resulting Source ClassModule is now:

Private mXYValues As XYDimensions

Property Let MyX(arg As Long)
    mXYValues.MyX = arg
End Property

Property Get MyX() As Long
    MyX = mXYValues.MyX
End Property

Property Let MyY(arg As Long)
    mXYValues.MyY = arg
End Property

Property Get MyY() As Long
    MyY = mXYValues.MyY
End Property

Public Function CalculateArea() As Long
   CalculateArea = mXYValues .MyX * mXYValues .MyY
End Function

Private Sub Class_Initialize()
    Set mXYValues = New XYDimensions
End Sub

This avoids all the complications of evaluating existing call sites and logic flow scenarios. And, the goal of 'moving' the behavior code to a new class is accomplished. Then, if the user wants to remove the moved member(s) from the source class, they can delete the members and take on case by case call-site scenarios.

[retailcoder]

@Bzgr I think the linked issue might be throwing a wrench into the class-to-class scenario, or warrant an interface-to-interface scenario (very much not trivial if already implemented). Thoughts?

[BZngr]

@retailcoder Sorry I missed this message so long ago (@Bzgr typo didn't find me!).

As to the linked issue of relocating portions of an existing oversized interface - here's my 25 cents:

The class-to-class move scenario (as described above) tries to avoid disturbing how the method is currently called from other modules - thus avoiding all sorts of instantiation adn logic-flow complexity. In the interface-to-interface scenario, the clients have to call the interface in a different manner, because that is the whole point of slicing up the interface. Subdividing interfaces has a lot in common with moving a member, but there are some aspects that are different. For subdividing interfaces, the classes implementing the original 'large' interface remain the implementers of the newly subdivided interfaces - but all the specific implementation code, state management, and logic are already present in the existing implementing classes - and will remain unchanged (in its entirety I suspect). Except in the simplest of scenarios, moving a member often wants to 'drag along' some state variables, private helper methods, and retain the ability to callback to the original module - I think this quick-fix would get a 'free pass' on that aspect.

Basically, I'm thinking the refactoring/quick-fix implementation process would include:

1. Move interface members to new class/interface modules
2. Add Implements statements and modify the implementation method signatures in each implementer
3. Introduce new instances of the moved interfaces at all the call sites as needed. - Lots of class/interface instantiate code to insert and method signatures/parameters to modify where the original interface may have been passed around via other methods and properties. The complexity of doing this (or rather, in the absence of finding a general solution) drove me to the above class-to-class approach...which is still a WIP.