Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract Method called, it wants its UI back #1599

Open
retailcoder opened this Issue May 27, 2016 · 5 comments

Comments

@retailcoder
Copy link
Member

retailcoder commented May 27, 2016

Selecting the body of the For loop here:

'@Folder("FizzBuzz")
Option Explicit

Private Const FIZZ As Integer = 3
Private Const BUZZ As Integer = 5

Public Sub RunFizzBuzz()
    Dim result As String
    Dim value As Integer

    For value = 1 To 100
        Select Case True
            Case value Mod (FIZZ * BUZZ) = 0
                result = "FIZZBUZZ"
            Case value Mod BUZZ = 0
                result = "BUZZ"
            Case value Mod FIZZ = 0
                result = "FIZZ"
            Case Else
                result = value
        End Select
        Debug.Print result
    Next

End Sub

Results in:

Public Sub RunFizzBuzz()
    Dim result As String
    Dim value As Integer

    For value = 1 To 100
NewMethod value, result
        Debug.Print result
    Next

End Sub
Private Sub NewMethod(ByRef value As Integer, ByRef result As String)
        Select Case True
            Case value Mod (FIZZ * BUZZ) = 0
                result = "FIZZBUZZ"
            Case value Mod BUZZ = 0
                result = "BUZZ"
            Case value Mod FIZZ = 0
                result = "FIZZ"
            Case Else
                result = value
        End Select
End Sub

I love the changes made to the refactoring under the hood! And it's FAST! But on the surface:

  • Users lose the ability to name extracted method
  • Users lose the ability to preview and confirm the refactoring
  • Users lose the ability to extract a Function and pick one of the possible return values
  • The extracted method should be indented, and call site should be inserted in the same column as the extracted code started
  • Previous versions put an empty line between the parent and the extracted members
  • Previous versions passed unassigned (input) parameters ByVal

Be it only for the ability to name the method before creating (and parsing) it, and preview/confirm the refactoring, Extract Method needs to have its UI back.

If the user is able to extract the method they actually want, the way they want it, then they don't need to tweak it after okaying the dialog, and so we need to parse less often.

Public Sub RunFizzBuzz()
    Dim result As String
    Dim value As Integer

    For value = 1 To 100
        result = FizzBuzz(value)
        Debug.Print result
    Next

End Sub

Private Function FizzBuzz(ByVal value As Integer) As String
    Dim result As String
    Select Case True
        Case value Mod (FIZZ * BUZZ) = 0
            result = "FIZZBUZZ"
        Case value Mod BUZZ = 0
            result = "BUZZ"
        Case value Mod FIZZ = 0
            result = "FIZZ"
        Case Else
            result = value
    End Select
    FizzBuzz = result
End Function

@retailcoder retailcoder added this to the Version 2.0 milestone May 27, 2016

@retailcoder retailcoder assigned grleachman and unassigned grleachman May 27, 2016

@grleachman

This comment has been minimized.

Copy link
Contributor

grleachman commented May 27, 2016

This should really read:
Gareth. You broke it, now fix it.

So assign it back to me, and I'll get to it once the edge cases work.

@Vogel612

This comment has been minimized.

Copy link
Member

Vogel612 commented Jul 30, 2016

What's the status on this? Is this still an issue?

@Hosch250

This comment has been minimized.

Copy link
Member

Hosch250 commented Jul 30, 2016

The UI is still there, but it doesn't display when Extract Method is called.

@bclothier

This comment has been minimized.

Copy link
Contributor

bclothier commented Nov 8, 2017

@Vogel612 assign to me and add to epic

@Vogel612 Vogel612 added this to TODO in Current Epic Nov 8, 2017

@Vogel612 Vogel612 added the enhancement label Dec 7, 2017

@bclothier bclothier moved this from TODO to In Progress in Current Epic Dec 12, 2017

@bclothier

This comment has been minimized.

Copy link
Contributor

bclothier commented Dec 12, 2017

Restoring dialog, implementing as WinForms containing WPF elementhost as per current standard is WIP. See PR #3588 and #3586.

@bclothier bclothier added the work-item label Aug 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.