Extract Method creates paramater & local variable of the same name.

[daFreeMan]

Rubberduck version information

Version 2.5.0.5338
OS: Microsoft Windows NT 10.0.17134.0, x64
Host Product: Microsoft Office 2016 x64
Host Version: 16.0.4924.1000
Host Executable: MSACCESS.EXE

Description Extracting some code (part of a CASE statement) yields a call passing a parameter that doesn't exist at the call site and a procedure parameter and local variable of the same name.

To Reproduce

1. Starting with this code selected (part of a CASE statement)

   'TODO refactor this case into its own routine
   Dim sqlString As String
   sqlString = "SELECT Column1 " & _
             "FROM table " & _
             "WHERE Column2 = " & someValue
   Dim clinicUsesNRC As Variant
   clinicUsesNRC = SQLQuerySingleString(sqlString)
   If clinicUsesNRC = vbNullString Then
   clinicUsesNRC = False
   End If
   clinicUsesNRC = CBool(clinicUsesNRC)
   LoadExcelDataRC = GetApptPlusByApi(clinicList.Keys(index), downloaderStatus)

2. Performing Refactor | Extract Method yields this at the call site:

   NewMethod clinicList, index, downloaderStatus, LoadExcelDataRC, clinicUsesNRC

3. And produces this code (formatting adjusted to prevent side-scrolling):

   Private Sub NewMethod(ByRef clinicList As Scripting.Dictionary, ByRef index As Long, 
                                     ByRef downloaderStatus As WebDownloaderStatus, 
                                     ByRef LoadExcelDataRC As Long, 
                                     ByRef clinicUsesNRC As Variant)
     Dim sqlString As String
   'TODO refactor this case into its own routine
     sqlString = "SELECT Column1 " & _
                 "FROM table " & _
                 "WHERE Column2 = " & someValue
     Dim clinicUsesNRC As Variant
     clinicUsesNRC = SQLQuerySingleString(sqlString)
     If clinicUsesNRC = vbNullString Then
       clinicUsesNRC = False
     End If
     clinicUsesNRC = CBool(clinicUsesNRC)
     LoadExcelDataRC = GetApptPlusByApi(clinicList.Keys(index), downloaderStatus)
   End Sub

4. Note that clinicUsesNRC is not declared prior to the call, so it's an unknown parameter at the call site.

5. clinicUsesNRC is the name of the last parameter in NewMethod

6. clinicUsesNRC is also declared as a new Variant within NewMethod

Expected behavior Variables that are passed to the new method should exist prior to the NewMethod call. Variables declared within NewMethod's signature should not also be declared locally.

Also, I note that all NewMethod parameters are declared ByRef. I presume that's to protect against a now possibly side-effecting method and that it's up to the user (or RD inspections) to determine if ByRef or ByVal are appropriate since we don't yet have CPA to make that determination for us.

Screenshots n/a

Logfile RubberduckLog.txt

Additional context I've not attempted an MCVE - this may well be situational.

Here's the original, complete CASE statement, prior to the refactor:

      Select Case FileType
        Case SnapSurvey
          GetSnapSurveyFile Downloader, clinicList.Keys(index), clinicList.Items(index), downloaderStatus
        Case ApptPlus
        'TODO refactor this case into its own routine
          Dim sqlString As String
      sqlString = "SELECT Column1 " & _
                  "FROM table " & _
                  "WHERE Column2 = " & someValue
          Dim clinicUsesNRC As Variant
          clinicUsesNRC = SQLQuerySingleString(sqlString)
          If clinicUsesNRC = vbNullString Then
            clinicUsesNRC = False
          End If
          clinicUsesNRC = CBool(clinicUsesNRC)
          LoadExcelDataRC = GetApptPlusByApi(clinicList.Keys(index), downloaderStatus)
      End Select

Here's the complete CASE statement after the refactor:

      Select Case FileType
        Case SnapSurvey
          GetSnapSurveyFile Downloader, clinicList.Keys(index), clinicList.Items(index), downloaderStatus
        Case ApptPlus
NewMethod clinicList, index, downloaderStatus, LoadExcelDataRC, clinicUsesNRC
      End Select

As a side note, it would be awesome if the duck would perform an indent on the procedure at the call site, and at on the newly created method. But that's really a feature request.

Note: #2202 might be related, but seems to have been fixed at this point

[retailcoder]

Thanks for the report, but that's a "won't-fix", as you know EM is unsupported right now.