rubberduck-vba / Rubberduck

Every programmer needs a rubberduck. COM add-in for the VBA & VB6 IDE (VBE).
https://rubberduckvba.com
GNU General Public License v3.0
1.91k stars 299 forks source link

Refactoring Tool "Move Closer to Usage" should honor indenter settings #4375

Open ChrisBrackett opened 6 years ago

ChrisBrackett commented 6 years ago

The refactoring tool "Move Closer to Usage" doesn't honor indenter settings. For example, when using the tool on variable varToMove:

Sub test()
    Dim varToMove                       As String ' indented
    Dim j                               As Long
    Debug.Print varToMove
End Sub

It ends up like this:

Sub test()
    Dim j                               As Long
    Dim varToMove As String ' not indented
    Debug.Print varToMove
End Sub

It would make sense to either have this tool honor the indenter settings or just keep the same indentation on the declaration line the same as before it was moved.

Version 2.2.0.3762 OS: Microsoft Windows NT 6.1.7601 Service Pack 1, x64 Host Product: Microsoft Outlook x86 Host Version: 16.0.4717.1000 Host Executable: OUTLOOK.EXE

Vogel612 commented 6 years ago

Not quite sure why this is tagged with 01-duckling. If the Refactoring would automatically call the indenter afterwards, that'd be fine. As it is described, the refactoring would need to examine the target context's surroundings for other variable declarations.

Doing so is a somewhat complex endeavour. Note that the current description assumes the target site will be at the end of a block of declarations.

What if the original code was the following?

Sub test()
    Dim varToMove        As String
    Dim j                As Long
    Debug.Print j
    Debug.Print varToMove
End Sub

I think the "align as-types" in the indenter only makes sense in declaration blocks. The whole idea of the "MoveCloserToUsage" refactoring goes counter to the notion of declaration blocks.

The following code looks pretty horrendous IMO:

Sub test()
    Dim j                As Long
    Debug.Print j
    Dim varToMove        As String
    Debug.Print varToMove
End Sub

Ditching the as-type alignment makes it aesthetically more pleasing:

Sub test()
    Dim j As Long
    Debug.Print j
    Dim varToMove As String
    Debug.Print varToMove
End Sub

The alternative of keeping the original indentation of the line also falls apart pretty quickly when the declaration is moved into a block and looks downright ridiculous when moving more than two nesting levels.

Sub test()
    Dim j As Long             As String
    With Me.Logger
        For j = 0 To 100 Step 1
    Dim varToMove             As String
            If (j Mod 3 = 0 And j Mod 5 = 0) Then
                varToMove = "FizzBuzz"
            Else If (j Mod 5 = 0) Then
                varToMove = "Buzz"
            Else If (j Mod 3 = 0) Then
                varToMove = "Fizz"
            Else 
                varToMove = CStr(j)
            End If
            .Write(varToMove)
        Next
    End With
End Sub
daFreeMan commented 6 years ago

Horrendous looking code is, however, up to the programmer. If someone wants their Ases aligned, but uses Move Closer to Usage, then it's on them that their code is... difficult to read. After making these changes, they may chose to disable the As Type alignment and re-run the indenter and be done with it.