21
\$\begingroup\$

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.

Rubberduck's "Code Inspections" toolbar, floating

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.

Rubberduck's Extract Method dialog, previewing a method extracted from the body of a FizzBuzz loop.

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:

per above description, showing the 'result' variable now as a 'ByRef' parameter

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<ExtractedParameter> 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<ExtractedParameter> Parameters { get; set; }

    IEnumerable<ExtractedParameter> Inputs { get; set; }
    IEnumerable<ExtractedParameter> Outputs { get; set; }
    IEnumerable<ExtractedParameter> 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();
}

The bulk of the logic is in the ExtractMethodPresenter class.

  • Parameters are all variables in use in the selected snippet.
  • If a variable is only ever used in the selected snippet, then its declaration is moved to the new method and it's not passed as a parameter.
  • If a variable is used after the generated method call, and there's only one of such variables, that variable is the default suggested return value.
  • If there's more than one, they are passed as ByRef parameters.

Here's my implementation:

public class ExtractMethodPresenter
{
    private readonly IExtractMethodDialog _view;

    private readonly IParseTree _parentMethodTree;
    private IDictionary<VisualBasic6Parser.AmbiguousIdentifierContext, ExtractedDeclarationUsage> _parentMethodDeclarations;

    private readonly IEnumerable<ExtractedParameter> _input;
    private readonly IEnumerable<ExtractedParameter> _output;
    private readonly IEnumerable<VisualBasic6Parser.AmbiguousIdentifierContext> _locals; 

    private readonly string _selectedCode;
    private readonly VBE _vbe;
    private readonly Selection _selection;

    public ExtractMethodPresenter(VBE vbe, IExtractMethodDialog dialog, IParseTree parentMethod, Selection selection)
    {
        _vbe = vbe;
        _selection = selection;

        _view = dialog;
        _parentMethodTree = parentMethod;
        _selectedCode = vbe.ActiveCodePane.CodeModule.get_Lines(selection.StartLine, selection.LineCount);

        _parentMethodDeclarations = ExtractMethodRefactoring.GetParentMethodDeclarations(parentMethod, selection);

        var input = _parentMethodDeclarations.Where(kvp => kvp.Value == ExtractedDeclarationUsage.UsedBeforeSelection).ToList();
        var output = _parentMethodDeclarations.Where(kvp => kvp.Value == ExtractedDeclarationUsage.UsedAfterSelection).ToList();

        _locals = _parentMethodDeclarations.Where(
            kvp => kvp.Value == ExtractedDeclarationUsage.UsedOnlyInSelection
                || kvp.Value == ExtractedDeclarationUsage.UsedAfterSelection
            ).Select(kvp => kvp.Key);

        _input = ExtractParameters(input);
        _output = ExtractParameters(output);
    }

    private IEnumerable<ExtractedParameter> ExtractParameters(IList<KeyValuePair<VisualBasic6Parser.AmbiguousIdentifierContext, ExtractedDeclarationUsage>> declarations)
    {
        var consts = declarations
            .Where(kvp => kvp.Key.Parent is VisualBasic6Parser.ConstSubStmtContext)
            .Select(kvp => kvp.Key.Parent)
            .Cast<VisualBasic6Parser.ConstSubStmtContext>()
            .Select(constant => new ExtractedParameter(
                constant.ambiguousIdentifier().GetText(),
                constant.asTypeClause() == null
                    ? Tokens.Variant
                    : constant.asTypeClause().type().GetText(),
                ExtractedParameter.PassedBy.ByVal));

        var variables = declarations
            .Where(kvp => kvp.Key.Parent is VisualBasic6Parser.VariableSubStmtContext)
            .Select(kvp => new ExtractedParameter(
                kvp.Key.GetText(),
                ((VisualBasic6Parser.VariableSubStmtContext) kvp.Key.Parent).asTypeClause() == null
                    ? Tokens.Variant
                    : ((VisualBasic6Parser.VariableSubStmtContext) kvp.Key.Parent).asTypeClause().type().GetText(),
                ExtractedParameter.PassedBy.ByVal));

        var arguments = declarations
            .Where(kvp => kvp.Key.Parent is VisualBasic6Parser.ArgContext)
            .Select(kvp => new ExtractedParameter(
                kvp.Key.GetText(),
                ((VisualBasic6Parser.ArgContext)kvp.Key.Parent).asTypeClause() == null
                    ? Tokens.Variant
                    : ((VisualBasic6Parser.ArgContext)kvp.Key.Parent).asTypeClause().type().GetText(),
                ExtractedParameter.PassedBy.ByVal));

        return consts.Union(variables.Union(arguments));
    }

    public void Show()
    {
        _view.MethodName = "Method1";
        _view.Inputs = _input.ToList();
        _view.Outputs = _output.Select(output => new ExtractedParameter(output.Name, output.TypeName, ExtractedParameter.PassedBy.ByRef)).ToList();
        _view.Locals = _locals.Select(variable => new ExtractedParameter(variable.GetText(), string.Empty, ExtractedParameter.PassedBy.ByVal)).ToList();

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

        _view.ReturnValues = returnValues;
        if (_output.Count() == 1)
        {
            _view.ReturnValue = _output.Single();
        }

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

        var result = _view.ShowDialog();
        if (result != DialogResult.OK)
        {
            return;
        }

        _vbe.ActiveCodePane.CodeModule.DeleteLines(_selection.StartLine, _selection.LineCount - 1);
        _vbe.ActiveCodePane.CodeModule.ReplaceLine(_selection.StartLine, GetMethodCall());

        _vbe.ActiveCodePane.CodeModule.AddFromString(GetExtractedMethod());
    }

    private void _view_RefreshPreview(object sender, EventArgs e)
    {
        var hasReturnValue = _view.ReturnValue != null && _view.ReturnValue.Name != "(none)";
        _view.CanSetReturnValue = 
            hasReturnValue && !IsValueType(_view.ReturnValue.TypeName);

        Preview();
    }

    private void Preview()
    {
        _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 != "(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 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
    };

    public static bool IsValueType(string typeName)
    {
        return ValueTypes.Contains(typeName);
    }

    [ComVisible(false)]
    private string GetExtractedMethod()
    {
        const string newLine = "\r\n";

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

        var isFunction = _view.ReturnValue != null && _view.ReturnValue.Name != "(none)";
        if (isFunction)
        {
            keyword = Tokens.Function;
            returnType = Tokens.As + ' ' + _view.ReturnValue.TypeName;
        }

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

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

        var localConsts = _locals.Select(e => e.Parent)
            .OfType<VisualBasic6Parser.ConstSubStmtContext>()
            .Select(e => "    " + Tokens.Const + ' ' + e.ambiguousIdentifier().GetText() + ' ' + e.asTypeClause().GetText() + " = " + e.valueStmt().GetText());
        var localVariables = _locals.Select(e => e.Parent)
            .OfType<VisualBasic6Parser.VariableSubStmtContext>()
            .Where(e => _view.Parameters.All(param => param.Name != e.ambiguousIdentifier().GetText()))
            .Select(e => "    " + Tokens.Dim + ' ' + e.ambiguousIdentifier().GetText() + ' ' + e.asTypeClause().GetText());
        var locals = string.Join(newLine, localConsts.Union(localVariables).ToArray());

        result += newLine + locals + newLine + _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;
    }
}

Then there's this smelly ExtractMethodRefactoring class:

public class ExtractMethodRefactoring
{
    public static IDictionary<VisualBasic6Parser.AmbiguousIdentifierContext, ExtractedDeclarationUsage> GetParentMethodDeclarations(IParseTree parseTree, Selection selection)
    {
        var declarations = parseTree.GetContexts<DeclarationListener, ParserRuleContext>(new DeclarationListener()).ToList();

        var constants = declarations.OfType<VisualBasic6Parser.ConstSubStmtContext>().Select(constant => constant.ambiguousIdentifier());
        var variables = declarations.OfType<VisualBasic6Parser.VariableSubStmtContext>().Select(variable => variable.ambiguousIdentifier());
        var arguments = declarations.OfType<VisualBasic6Parser.ArgContext>().Select(arg => arg.ambiguousIdentifier());

        var identifiers = constants.Union(variables)
                                   .Union(arguments)
                                   .ToDictionary(declaration => declaration.GetText(), 
                                                 declaration => declaration);

        var references = parseTree.GetContexts<VariableReferencesListener, VisualBasic6Parser.AmbiguousIdentifierContext>(new VariableReferencesListener())
                                  .GroupBy(usage => new { Identifier = usage.GetText()})
                                  .ToList();

        var notUsedInSelection = references.Where(usage => usage.All(token => !selection.Contains(token.GetSelection())))
                                           .Select(usage => usage.Key).ToList();

        var usedBeforeSelection = references.Where(usage => usage.Any(token => token.GetSelection().EndLine < selection.StartLine))
                                                .Select(usage => usage.Key)
                                            .Where(usage => notUsedInSelection.All(e => e.Identifier != usage.Identifier));

        var usedAfterSelection = references.Where(usage => usage.Any(token => token.GetSelection().StartLine > selection.EndLine))
                                               .Select(usage => usage.Key)
                                            .Where(usage => notUsedInSelection.All(e => e.Identifier != usage.Identifier));

        var usedOnlyWithinSelection = references.Where(usage => usage.All(token => selection.Contains(token.GetSelection())))
                                                .Select(usage => usage.Key);


        var result = new Dictionary<VisualBasic6Parser.AmbiguousIdentifierContext, ExtractedDeclarationUsage>();

        // temporal coupling: references used after selection must be added first
        foreach (var reference in usedAfterSelection)
        {
            VisualBasic6Parser.AmbiguousIdentifierContext key;
            if (identifiers.TryGetValue(reference.Identifier, out key))
            {
                if (!result.ContainsKey(key))
                {
                    result.Add(key, ExtractedDeclarationUsage.UsedAfterSelection);
                }
            }
        }

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

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

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

        return result;
    }
}

I use this extension method quite a lot, to walk a parse tree whenever I need to:

public static class AntlrExtensions
{
    public static IEnumerable<TContext> GetContexts<TListener, TContext>(this IParseTree parseTree, TListener listener)
        where TListener : IExtensionListener<TContext>, IParseTreeListener
        where TContext : class
    {
        var walker = new ParseTreeWalker();
        walker.Walk(listener, parseTree);

        return listener.Members;
    }
}

Obviously I need to break this code into smaller pieces. Where to start?

Here is the ExtractedParameter class, for context:

public class ExtractedParameter
{
    public enum PassedBy
    {
        ByRef,
        ByVal
    }

    public ExtractedParameter(string name, string typeName, PassedBy passed)
    {
        Name = name;
        TypeName = typeName;
        Passed = passed;
    }

    public string Name { get; set; }
    public string TypeName { get; set; }
    public PassedBy Passed { get; set; }

    public override string ToString()
    {
        return Passed.ToString() + ' ' + Name + ' ' + Tokens.As + ' ' + TypeName;
    }
}
\$\endgroup\$
10
+250
\$\begingroup\$

Your GetParentMethodDeclarations() method has a lot of repeated code when it loops over the positionSelection collections. Considering all of them are the same, you can extract them to a helper method and do it once there.

At the same time I would rename result as well, not in the least because that would've made it easier for a poor reviewer like me to choose a name for the helper method.

private void ExtractDeclarationUsage(Dictionary<VisualBasic6Parser.AmbiguousIdentifierContext, ExtractedDeclarationUsage> declarationUsage,
                                     SomeBloodyType identifiers,
                                     IEnumerable<VisualBasic6Parser.AmbiguousIdentifierContext> identifierContexts, 
                                     ExtractedDeclarationUsage declarationUsage)
{
    foreach (var reference in identifierContexts)
    {
        VisualBasic6Parser.AmbiguousIdentifierContext key;
        if (identifiers.TryGetValue(reference.Identifier, out key))
        {
            if (!declarationUsage.ContainsKey(key))
            {
                declarationUsage.Add(key, declarationUsage);
            }
        }
    }
}

You'll notice that I had some trouble finding out what types were used exactly. It might be worth it to consider dropping var in cases where it isn't obvious.


ExtractedParameter seems like something that should be immutable. I would make its properties private set so people won't manipulate semantic info about the code.


I'm a little put off by the fully qualified VisualBasic6Parser namespace that's spread throughout the code. Can't this be omitted?


VariableSubStmtContext

Only heathens shorten their class names.


In ExtractParameters() you repeat a lot of complicated code for the variables and arguments collections when their only difference is the type used in the where clause and the cast. I would look into extracting this out to a method so you don't have this as complicated.

I haven't actually implemented this myself yet but this should get you in the right direction:

https://msdn.microsoft.com/en-us/library/bb882637.aspx

For the cast you can use Convert.ChangeType.


if (_output.Count() == 1)

This will prove worrisome if you have 20 brazillion items in _output. What I would suggest is this:

if(_output.Any() && !_output.Skip(1).Any())

Code speaks for itself: Check if it actually contains elements and disregard the first item and see if there are any others left.

It's a little more verbose but you can always create your own extension method like this:

public static bool HasOne<T>(this IEnumerable<T> enumerable) {
    var enumerator = enumerable.GetEnumerator();
    return enumerator.MoveNext() && !enumerator.MoveNext();
}

as described here.


I don't like this:

var returnValues = new[] { new ExtractedParameter("(none)", string.Empty, ExtractedParameter.PassedBy.ByVal) }
  • A return value is not a parameter so that is an inherent issue
  • You're using a magic string value "(none)" to indicate it doesn't have a name. I would prefer you to just make Name optional.
  • Same goes for TypeName but instead of using System.Void you decide on an empty string. Not a fan of that either.
  • I would say that return values are passed by reference-by-value since you can modify the backing objects that are returned from a method.

Altogether I would prefer to see returnValues get its own type because right now it's just sort of being forced in there.


Your event handling's naming is switched here: _view.RefreshPreview += _view_RefreshPreview; _view.OnRefreshPreview(); should be _view.OnRefreshPreview += _view_RefreshPreview; _view.RefreshPreview ();

Your method is named Show() but from the context (being class ExtractMethodPresenter) I can't really determine what is going to be shown.

Likewise Preview() should probably be ShowPreview() or something.


const string newLine = "\r\n";

We have Environment.NewLine for that.


\$\endgroup\$
  • \$\begingroup\$ VisualBasic6Parser isn't a namespace, it's a class generated by compiling the ANTLR grammar file - then there's all its nested generated ParserRuleContext classes. I've renamed the grammar to VB, so now I get a shorter VBParser class... but there's no avoiding it, ... or perhaps I could using-alias them.. But I won't escape the nested types! \$\endgroup\$ – Mathieu Guindon Feb 22 '15 at 13:03
  • \$\begingroup\$ "A" return value isn't a parameter, agreed; but in this case the extracted method's return value can be any of the extracted parameters... or none, depending on user's decision. It's funny you mention event-raising method names, I have a convention of systematically naming them On[EventName], as I thought that was the recommended convention (I even had to adapt to it!). All in all this answer is my favorite one, as it addresses the main pain point and things I wouldn't have thought of, even if some points are unapplicable in context (like the VisualBasic6 "namespace"). Well done! \$\endgroup\$ – Mathieu Guindon Feb 24 '15 at 0:39
  • 1
    \$\begingroup\$ "Only Heathens shorten their class names." Lol. Yes yes. True. It's a good thing that's a generated class. =) \$\endgroup\$ – RubberDuck Mar 18 '15 at 22:05
  • \$\begingroup\$ Actually, with respect to event naming, the convention is that an event is a verb (sometimes past tense)(e.g., Click or EnabledChanged) and the method responsible for raising the event prefixes the event name with On (e.g., OnClick and OnEnabledChanged). For real-world examples, see System.Windows.Forms.Control or for you WPFers, System.Windows.Controls.Control. \$\endgroup\$ – Dan Lyons May 14 '15 at 17:56
  • \$\begingroup\$ @DanLyons: hmm, seems like you're right. I'm not sure I agree with this convention but so be it. \$\endgroup\$ – Jeroen Vannevel May 14 '15 at 18:17
10
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Why declare the key outside the loops? \$\endgroup\$ – janos Feb 22 '15 at 7:54
  • \$\begingroup\$ I prefer the nested if statements. There's nothing gained by putting it on the same line except obscuring it a little more because now that condition is more hidden. \$\endgroup\$ – Jeroen Vannevel Feb 22 '15 at 11:52
  • \$\begingroup\$ @JeroenVannevel They don't necessarily have to be on the same line, I just think they should be in the same if. \$\endgroup\$ – 23fc9a62-56de-47fb-97b4-737890 Feb 22 '15 at 16:07
  • \$\begingroup\$ @janos If you declare key outside the loops, you don't need to keep creating and destroying it. Also, it better shows that it is used by the rest of the code in the method. \$\endgroup\$ – 23fc9a62-56de-47fb-97b4-737890 Feb 22 '15 at 16:09
  • 1
    \$\begingroup\$ @Hosch250 it's good to declare variables in the smallest scope where they are used. And it's good to not reuse variable for multiple purposes. There is nothing to gain by moving this variable outside the loop, and moving it outside can lead to misuse by accident, confusion and errors. The code is cleaner with that variable declared inside, and becomes less clean if you move it outside. \$\endgroup\$ – janos Feb 22 '15 at 16:49
8
\$\begingroup\$

Reduce variable span and live time

Let me quote an interesting paragraph from Code Complete:

The code between references to a variable is a “window of vulnerability.” In the window, new code might be added, inadvertently altering the variable, or someone reading the code might forget the value the variable is supposed to contain. It’s always a good idea to localize references to variables by keeping them close together.

Indicators of this window of vulnerability are the measurements of variable "span" and "live time":

  • Variable "span": the number of lines between references to a variable. When a variable is referenced multiple times, the average span is computed by averaging all the individual spans. The smaller the better.

  • Variable "live time": the total number of statements over which a variable is "live". This is a count of lines between the first reference and the last. Again, the smaller the better.

In this kind of code, you have those metrics up:

var usedAfterSelection = ...

var usedBeforeSelection = ...

var result = new Dictionary<>();

foreach (var reference in usedAfterSelection) { ... }

foreach (var reference in usedBeforeSelection) { ... }

This would be better:

var result = new Dictionary<>();

var usedAfterSelection = ...

foreach (var reference in usedAfterSelection) { ... }

var usedBeforeSelection = ...

foreach (var reference in usedBeforeSelection) { ... }

Both the span and the live time of usedBeforeSelection and usedAfterSelection are reduced, and so is their window of vulnerability.

At this point, it's also quite clear that the pairs of actions (create collection -> iterate over the elements) could be extracted to helper methods. That will make the code more readable: when you look at the body of ExtractMethodRefactoring, it will look like the list of big steps, and you can see the ordering of these steps better too.

You will also get the usual benefit of decomposing to smaller methods: you will be able to test these elements piece by piece.

\$\endgroup\$

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.