Rubberduck's "Extract Method" refactoring implementation

With the ANTLR-powered parser, I was able to reimplement all code inspections from the last release build, and the rest of the inspections on the road map are now implementable.

Code inspections and quick-fixes are nice, but we want our Rubberduck even smarter than that. So I wrote one of the coolest things I've ever implemented - an Extract Method refactoring functionality!

Problem is, I'm pretty green working with an ANTLR parse tree, and I don't know if I'm doing this right. It works, but perhaps I'm doing dumb things that slow things down or something. Also as much as I'm proud of what this code does, it looks... horrible.

Changing the return value automatically updates the preview box. For example, if we decided we didn't want to return anything, the signature would change from Function to Sub, and result would be passed as a ByRef parameter instead of being a local variable to return:

The only thing it doesn't do [yet] I wish it did, is cleaning up unused declarations in the original method... but I can live with that for a few more releases.

The view is consumed as an interface:

public interface IExtractMethodDialog : IDialogView
{
    ExtractedParameter ReturnValue { get; set; }
    IEnumerable ReturnValues { get; set; }

    VBAccessibility Accessibility { get; set; }
    string MethodName { get; set; }
    bool SetReturnValue { get; set; }
    bool CanSetReturnValue { get; set; }

    event EventHandler RefreshPreview;
    void OnRefreshPreview();
    string Preview { get; set; }

    IEnumerable Parameters { get; set; }

    IEnumerable Inputs { get; set; }
    IEnumerable Outputs { get; set; }
    IEnumerable Locals { get; set; }
}

The IDialogView interface it inherits is just a bare-bones dialog interface:

public interface IDialogView
{
    event EventHandler CancelButtonClicked;
    void OnCancelButtonClicked();

    event EventHandler OkButtonClicked;
    void OnOkButtonClicked();

    DialogResult ShowDialog();
}

Th

First off, I noticed this in ExtractMethodRefactoring:

if (identifiers.TryGetValue(reference.Identifier, out key))
{
    if (!result.ContainsKey(key))
    {
        result.Add(key, ExtractedDeclarationUsage.UsedAfterSelection);
    }
}

That should be one if:

if (identifiers.TryGetValue(reference.Identifier, out key) && !result.ContainsKey(key))
{
    result.Add(key, ExtractedDeclarationUsage.UsedAfterSelection);
}

You have this several times through out the class.

The set of loops are nearly identical:

foreach (var reference in usedAfterSelection)
{
    VisualBasic6Parser.AmbiguousIdentifierContext key;
    if (identifiers.TryGetValue(reference.Identifier, out key))
    {
        if (!result.ContainsKey(key))
        {
            result.Add(key, ExtractedDeclarationUsage.UsedAfterSelection);
        }
    }
}

Why not create a method for this and pass the necessary values? You would have to pass several values, so this might be a pain, but it would have less repetition than this way.

StackExchange Code Review Q#80568, answer score: 10