patterncsharpMinor

Extract Method: A refactored refactoring

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

refactoringrefactoredmethodextract

Problem

This post is somewhat of a follow-up to Rubberduck's "Extract Method" refactoring implementation.

A tremendous amount of things have changed since version 1.2, when this feature was first introduced - the more important thing being the advent of the Declarations API, which implies that the feature now not only gets an ANTLR parse tree to play with, but also a strongly-typed IEnumerable collection, each with their References resolved (see this post for more details on that specific part).

This means I no longer need to walk the parse tree, so I removed a tremendous amount of code here, and simplified the "who's used where" logic in unspeakable ways.

Thing is, I'm sure it can still be tremendously improved.

```
namespace Rubberduck.UI.Refactorings.ExtractMethod
{
public class ExtractMethodPresenter
{
private readonly IExtractMethodDialog _view;

private readonly IEnumerable _input;
private readonly IEnumerable _output;
private readonly List _locals;
private readonly List _toRemoveFromSource;

private readonly string _selectedCode;
private readonly QualifiedSelection _selection;

private readonly IActiveCodePaneEditor _editor;
private readonly Declaration _member;

private readonly HashSet _usedInSelection;
private readonly HashSet _usedBeforeSelection;
private readonly HashSet _usedAfterSelection;

public ExtractMethodPresenter(IActiveCodePaneEditor editor, IExtractMethodDialog view, Declaration member, QualifiedSelection selection, Declarations declarations)
{
_editor = editor;
_view = view;
_member = member;
_selection = selection;

_selectedCode = _editor.GetLines(selection.Selection);

var inScopeDeclarations = declarations.Items.Where(item => item.ParentScope == member.Scope).ToList();

var inSelection = inScopeDeclarations.SelectMany(item => item.Refer

Solution

That constructor looks kind of messy. What I would do here is create a field _declarations and assign it in the constructor:

public ExtractMethodPresenter(IActiveCodePaneEditor editor, IExtractMethodDialog view, Declaration member, QualifiedSelection selection, Declarations declarations)
{
    _editor = editor;
    _view = view;
    _member = member;
    _selection = selection;
    _declarations = declarations;

    InitializeFields();
}

InitializeFields might look something like this:

private void InitializeFields()
{
    _selectedCode = _editor.GetLines(selection.Selection);

    var inScopeDeclarations = _declarations.Items.Where(item => item.ParentScope == member.Scope).ToList();

    var inSelection = inScopeDeclarations.SelectMany(item => item.References)
                                         .Where(item => _selection.Selection.Contains(item.Selection))
                                         .ToList();

    _usedInSelection = new HashSet(inScopeDeclarations.Where(item =>
        item.References.Any(reference => inSelection.Contains(reference))));

    _usedBeforeSelection = new HashSet(inScopeDeclarations.Where(item => 
        item.References.Any(reference => reference.Selection.StartLine (inScopeDeclarations.Where(item =>
        item.References.Any(reference => reference.Selection.StartLine > _selection.Selection.EndLine)));

    // identifiers used inside selection and before selection (or if it's a parameter) are candidates for parameters:
    var input = inScopeDeclarations.Where(item => 
        _usedInSelection.Contains(item) && (_usedBeforeSelection.Contains(item) || item.DeclarationType == DeclarationType.Parameter)).ToList();

    // identifiers used inside selection and after selection are candidates for return values:
    var output = inScopeDeclarations.Where(item => 
        _usedInSelection.Contains(item) && _usedAfterSelection.Contains(item))
        .ToList();

    // identifiers used only inside and/or after selection are candidates for locals:
    _locals = inScopeDeclarations.Where(item => item.DeclarationType != DeclarationType.Parameter && (
        item.References.All(reference => inSelection.Contains(reference))
        || (_usedAfterSelection.Contains(item) && (!_usedBeforeSelection.Contains(item)))))
        .ToList();

    // locals that are only used in selection are candidates for being moved into the new method:
    _toRemoveFromSource = _locals.Where(item => !_usedAfterSelection.Contains(item)).ToList();

    _output = output.Select(declaration =>
        new ExtractedParameter(declaration.AsTypeName, ExtractedParameter.PassedBy.ByRef, declaration.IdentifierName));

    _input = input.Where(declaration => !output.Contains(declaration))
        .Select(declaration =>
            new ExtractedParameter(declaration.AsTypeName, ExtractedParameter.PassedBy.ByVal, declaration.IdentifierName));
}

Code Snippets

public ExtractMethodPresenter(IActiveCodePaneEditor editor, IExtractMethodDialog view, Declaration member, QualifiedSelection selection, Declarations declarations)
{
    _editor = editor;
    _view = view;
    _member = member;
    _selection = selection;
    _declarations = declarations;

    InitializeFields();
}

private void InitializeFields()
{
    _selectedCode = _editor.GetLines(selection.Selection);

    var inScopeDeclarations = _declarations.Items.Where(item => item.ParentScope == member.Scope).ToList();

    var inSelection = inScopeDeclarations.SelectMany(item => item.References)
                                         .Where(item => _selection.Selection.Contains(item.Selection))
                                         .ToList();

    _usedInSelection = new HashSet<Declaration>(inScopeDeclarations.Where(item =>
        item.References.Any(reference => inSelection.Contains(reference))));

    _usedBeforeSelection = new HashSet<Declaration>(inScopeDeclarations.Where(item => 
        item.References.Any(reference => reference.Selection.StartLine < _selection.Selection.StartLine)));

    _usedAfterSelection = new HashSet<Declaration>(inScopeDeclarations.Where(item =>
        item.References.Any(reference => reference.Selection.StartLine > _selection.Selection.EndLine)));

    // identifiers used inside selection and before selection (or if it's a parameter) are candidates for parameters:
    var input = inScopeDeclarations.Where(item => 
        _usedInSelection.Contains(item) && (_usedBeforeSelection.Contains(item) || item.DeclarationType == DeclarationType.Parameter)).ToList();

    // identifiers used inside selection and after selection are candidates for return values:
    var output = inScopeDeclarations.Where(item => 
        _usedInSelection.Contains(item) && _usedAfterSelection.Contains(item))
        .ToList();

    // identifiers used only inside and/or after selection are candidates for locals:
    _locals = inScopeDeclarations.Where(item => item.DeclarationType != DeclarationType.Parameter && (
        item.References.All(reference => inSelection.Contains(reference))
        || (_usedAfterSelection.Contains(item) && (!_usedBeforeSelection.Contains(item)))))
        .ToList();

    // locals that are only used in selection are candidates for being moved into the new method:
    _toRemoveFromSource = _locals.Where(item => !_usedAfterSelection.Contains(item)).ToList();

    _output = output.Select(declaration =>
        new ExtractedParameter(declaration.AsTypeName, ExtractedParameter.PassedBy.ByRef, declaration.IdentifierName));

    _input = input.Where(declaration => !output.Contains(declaration))
        .Select(declaration =>
            new ExtractedParameter(declaration.AsTypeName, ExtractedParameter.PassedBy.ByVal, declaration.IdentifierName));
}

Context

StackExchange Code Review Q#90704, answer score: 3

Revisions (0)

No revisions yet.