10
\$\begingroup\$

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<Declaration> 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<ExtractedParameter> _input;
        private readonly IEnumerable<ExtractedParameter> _output;
        private readonly List<Declaration> _locals;
        private readonly List<Declaration> _toRemoveFromSource;

        private readonly string _selectedCode;
        private readonly QualifiedSelection _selection;

        private readonly IActiveCodePaneEditor _editor;
        private readonly Declaration _member;

        private readonly HashSet<Declaration> _usedInSelection;
        private readonly HashSet<Declaration> _usedBeforeSelection;
        private readonly HashSet<Declaration> _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.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));
        }

        public void Show()
        {
            PrepareView();
            if (_view.ShowDialog() != DialogResult.OK)
            {
                return;
            }

            ExtractMethod();
        }

        private void PrepareView()
        {
            _view.MethodName = "Method1";
            _view.Inputs = _input.ToList();
            _view.Outputs = _output.ToList();
            _view.Locals =
                _locals.Select(
                    variable =>
                        new ExtractedParameter(variable.AsTypeName, ExtractedParameter.PassedBy.ByVal, variable.IdentifierName))
                    .ToList();

            var returnValues = new[] {new ExtractedParameter(string.Empty, ExtractedParameter.PassedBy.ByVal)}
                .Union(_view.Outputs)
                .Union(_view.Inputs)
                .ToList();

            _view.ReturnValues = returnValues;
            _view.ReturnValue = (_output.Any() && !_output.Skip(1).Any())
                ? _output.Single()
                : returnValues.First();

            _view.RefreshPreview += _view_RefreshPreview;
            _view.OnRefreshPreview();
        }

        private void ExtractMethod()
        {
            _editor.DeleteLines(_selection.Selection);
            _editor.ReplaceLine(_selection.Selection.StartLine, GetMethodCall());

            var insertionLine = _member.Context.GetSelection().EndLine - _selection.Selection.LineCount + 2;
            _editor.InsertLines(insertionLine, GetExtractedMethod());

            // assumes these are declared *before* the selection...
            var offset = 0;
            foreach (var declaration in _toRemoveFromSource.OrderBy(e => e.Selection.StartLine))
            {
                var target = new Selection(
                    declaration.Selection.StartLine - offset,
                    declaration.Selection.StartColumn,
                    declaration.Selection.EndLine - offset,
                    declaration.Selection.EndColumn);

                _editor.DeleteLines(target);
                offset += declaration.Selection.LineCount;
            }
        }

        private static readonly IEnumerable<string> ValueTypes = new[]
        {
            Tokens.Boolean,
            Tokens.Byte,
            Tokens.Currency,
            Tokens.Date,
            Tokens.Decimal,
            Tokens.Double,
            Tokens.Integer,
            Tokens.Long,
            Tokens.LongLong,
            Tokens.Single,
            Tokens.String
        };

        private void _view_RefreshPreview(object sender, EventArgs e)
        {
            var hasReturnValue = _view.ReturnValue != null && _view.ReturnValue.Name != ExtractedParameter.None;
            _view.CanSetReturnValue =
                hasReturnValue && !ValueTypes.Contains(_view.ReturnValue.TypeName);

            GeneratePreview();
        }

        private void GeneratePreview()
        {
            _view.Preview = GetExtractedMethod();
        }

        private string GetMethodCall()
        {
            string result;
            var returnValueName = _view.ReturnValue.Name;
            var argsList = string.Join(", ", _view.Parameters.Select(p => p.Name));
            if (returnValueName != ExtractedParameter.None)
            {
                var setter = _view.SetReturnValue ? Tokens.Set + ' ' : string.Empty;
                result = setter + returnValueName + " = " + _view.MethodName + '(' + argsList + ')';
            }
            else
            {
                result = _view.MethodName + ' ' + argsList;
            }

            return "    " + result; // todo: smarter indentation
        }

        private string GetExtractedMethod()
        {
            var newLine = Environment.NewLine;

            var access = _view.Accessibility.ToString();
            var keyword = Tokens.Sub;
            var asTypeClause = string.Empty;

            var isFunction = _view.ReturnValue != null && _view.ReturnValue.Name != ExtractedParameter.None;
            if (isFunction)
            {
                keyword = Tokens.Function;
                asTypeClause = Tokens.As + ' ' + _view.ReturnValue.TypeName;
            }

            var parameters = "(" + string.Join(", ", _view.Parameters) + ")";

            var result = access + ' ' + keyword + ' ' + _view.MethodName + parameters + ' ' + asTypeClause + newLine;

            var localConsts = _locals.Where(e => e.DeclarationType == DeclarationType.Constant)
                .Cast<ValuedDeclaration>()
                .Select(e => "    " + Tokens.Const + ' ' + e.IdentifierName + ' ' + Tokens.As + ' ' + e.AsTypeName + " = " + e.Value);

            var localVariables = _locals.Where(e => e.DeclarationType == DeclarationType.Variable)
                .Where(e => _view.Parameters.All(param => param.Name != e.IdentifierName))
                .Select(e => e.Context)
                .Cast<VBAParser.VariableSubStmtContext>()
                .Select(e => "    " + Tokens.Dim + ' ' + e.ambiguousIdentifier().GetText() + 
                    (e.LPAREN() == null 
                        ? string.Empty 
                        : e.LPAREN().GetText() + (e.subscripts() == null ? string.Empty : e.subscripts().GetText()) + e.RPAREN().GetText()) + ' ' + 
                        (e.asTypeClause() == null ? string.Empty : e.asTypeClause().GetText()));
            var locals = string.Join(newLine, localConsts.Union(localVariables)
                            .Where(local => !_selectedCode.Contains(local)).ToArray()) + newLine;

            result += locals + _selectedCode + newLine;

            if (isFunction)
            {
                // return value by assigning the method itself:
                var setter = _view.SetReturnValue ? Tokens.Set + ' ' : string.Empty;
                result += "    " + setter + _view.MethodName + " = " + _view.ReturnValue.Name + newLine;
            }

            result += Tokens.End + ' ' + keyword + newLine;

            return newLine + result + newLine;
        }
    }
}

The good news is that this rewrite solves a number of issues that have been identified since the initial release of the feature... but a number of things jump at me in the constructor and fields, and I'm particularly interested in the following:

  • I feel like the constructor is doing too much work. Should I move its logic elsewhere?
  • GetExtractedMethod is... bleh. How can I turn this ugliness into something pretty?
  • Anything else?

Note: The project uses an grammar to generate a parser; the generated class is called VBAParser, and has a nested class for each grammar rule (e.g. VBAParser.VariableSubStmtContext). As much as I'd love these rules to follow C# naming conventions, it isn't something that's possible. Parser rules are camelCase, and lexer tokens are YELLCASE; that's just how it is.


For context, here's the ExtractedParameter class:

namespace Rubberduck.UI.Refactorings.ExtractMethod
{
    public class ExtractedParameter
    {
        public enum PassedBy
        {
            ByRef,
            ByVal
        }

        public static readonly string None = RubberduckUI.ExtractMethod_OutputNone;

        private readonly string _name;
        private readonly string _typeName;
        private readonly PassedBy _passedBy;

        public ExtractedParameter(string typeName, PassedBy passedBy, string name = null)
        {
            _name = name ?? None;
            _typeName = typeName;
            _passedBy = passedBy;
        }

        public string Name
        {
            get { return _name; }
        }

        public string TypeName
        {
            get { return _typeName; }
        }

        public override string ToString()
        {
            return _passedBy.ToString() + ' ' + Name + ' ' + Tokens.As + ' ' + TypeName;
        }
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ I love "YELLCASE". haven't seen that before. \$\endgroup\$ – Rossco May 15 '15 at 4:10
3
\$\begingroup\$

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<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));
}
\$\endgroup\$
  • \$\begingroup\$ There doesn't seem to be a need to keep a reference to declarations. Why not just pass it along to InitializeFields? I.e. InitializeFields(declarations) \$\endgroup\$ – RubberDuck May 17 '15 at 18:01
  • 1
    \$\begingroup\$ @RubberDuck That is an idea too. However, this variable can be very large, so you might want to pass by reference. \$\endgroup\$ – Hosch250 May 17 '15 at 18:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.