18
\$\begingroup\$

My latest refactoring for Rubberduck is called Introduce Field - it promotes a local variable to a private field.

The three overridden Refactor() methods are the members of IRefactoring, and are used to start the refactoring sequence. The other methods are the worker methods.

Overall, I am pretty happy with it, but there are a few things that bother me. The first of these is that Refactor(Declaration) has a strict requirement for the declaration type, but accepts any declaration, regardless of the type. Is there a way to enforce this other than throwing? Should I even be throwing here? Should I just return instead?

RemoveVariable() and RemoveExtraComma() both seem somewhat clunky. Is there a cleaner way to do this?

public class IntroduceField : IRefactoring
{
    private readonly IList<Declaration> _declarations;
    private readonly IActiveCodePaneEditor _editor;
    private Declaration _targetDeclaration;
    private readonly IMessageBox _messageBox;

    public IntroduceField(RubberduckParserState parseResult, IActiveCodePaneEditor editor, IMessageBox messageBox)
    {
        _declarations = parseResult.AllDeclarations.ToList();
        _editor = editor;
        _messageBox = messageBox;
    }

    public void Refactor()
    {
        if (_targetDeclaration == null)
        {
            _messageBox.Show(RubberduckUI.PromoteVariable_InvalidSelection);
            return;
        }

        RemoveVariable();
        AddField();
    }

    public void Refactor(QualifiedSelection selection)
    {
        _targetDeclaration = FindSelection(selection);
        Refactor();
    }

    public void Refactor(Declaration target)
    {
        if (target.DeclarationType != DeclarationType.Variable)
        {
            throw new ArgumentException("Invalid declaration type");
        }

        _targetDeclaration = target;
        Refactor();
    }

    private void AddField()
    {
        var module = _targetDeclaration.QualifiedName.QualifiedModuleName.Component.CodeModule;
        module.InsertLines(module.CountOfDeclarationLines + 1, GetFieldDefinition());
    }

    private void RemoveVariable()
    {
        Selection selection;
        var declarationText = _targetDeclaration.Context.GetText();
        var multipleDeclarations = HasMultipleDeclarationsInStatement(_targetDeclaration);

        var variableStmtContext = GetVariableStmtContext(_targetDeclaration);

        if (!multipleDeclarations)
        {
            declarationText = variableStmtContext.GetText();
            selection = GetVariableStmtContextSelection(_targetDeclaration);
        }
        else
        {
            selection = new Selection(_targetDeclaration.Context.Start.Line, _targetDeclaration.Context.Start.Column,
                _targetDeclaration.Context.Stop.Line, _targetDeclaration.Context.Stop.Column);
        }

        var oldLines = _editor.GetLines(selection);

        var newLines = oldLines.Replace(" _" + Environment.NewLine, string.Empty)
            .Remove(selection.StartColumn, declarationText.Length);

        if (multipleDeclarations)
        {
            selection = GetVariableStmtContextSelection(_targetDeclaration);
            newLines = RemoveExtraComma(_editor.GetLines(selection).Replace(oldLines, newLines));
        }

        _editor.DeleteLines(selection);
        _editor.InsertLines(selection.StartLine, newLines);
    }

    private Selection GetVariableStmtContextSelection(Declaration target)
    {
        var statement = GetVariableStmtContext(target);

        return new Selection(statement.Start.Line, statement.Start.Column,
                statement.Stop.Line, statement.Stop.Column);
    }

    private VBAParser.VariableStmtContext GetVariableStmtContext(Declaration target)
    {
        var statement = target.Context.Parent.Parent as VBAParser.VariableStmtContext;
        if (statement == null)
        {
            throw new MissingMemberException("Statement not found");
        }

        return statement;
    }

    private string RemoveExtraComma(string str)
    {
        if (str.Count(c => c == ',') == 1)
        {
            return str.Remove(str.IndexOf(','), 1);
        }

        var significantCharacterAfterComma = false;

        for (var index = 0; index < str.Length; index++)
        {
            if (!char.IsWhiteSpace(str[index]) && str[index] != '_' && str[index] != ',')
            {
                significantCharacterAfterComma = true;
            }
            if (str[index] == ',')
            {
                significantCharacterAfterComma = false;
            }

            if (!significantCharacterAfterComma && str[index] == ',')
            {
                return str.Remove(index, 1);
            }
        }

        return str;
    }

    private bool HasMultipleDeclarationsInStatement(Declaration target)
    {
        var statement = target.Context.Parent as VBAParser.VariableListStmtContext;

        if (statement == null) { return false; }

        return statement.children.Count(i => i is VBAParser.VariableSubStmtContext) > 1;
    }

    private string GetFieldDefinition()
    {
        if (_targetDeclaration == null) { return null; }

        return "Private " + _targetDeclaration.IdentifierName + " As " + _targetDeclaration.AsTypeName;
    }

    private Declaration FindSelection(QualifiedSelection selection)
    {
        var target = _declarations
            .Where(item => !item.IsBuiltIn)
            .FirstOrDefault(item => item.IsSelected(selection) && item.DeclarationType == DeclarationType.Variable
                                 || item.References.Any(r => r.IsSelected(selection) &&
                                    r.Declaration.DeclarationType == DeclarationType.Variable));

        if (target != null) { return target; }

        var targets = _declarations
            .Where(item => !item.IsBuiltIn
                           && item.ComponentName == selection.QualifiedName.ComponentName
                           && item.DeclarationType == DeclarationType.Variable);

        foreach (var declaration in targets)
        {
            var declarationSelection = new Selection(declaration.Context.Start.Line,
                                                declaration.Context.Start.Column,
                                                declaration.Context.Stop.Line,
                                                declaration.Context.Stop.Column + declaration.Context.Stop.Text.Length);

            if (declarationSelection.Contains(selection.Selection) ||
                !HasMultipleDeclarationsInStatement(declaration) && GetVariableStmtContextSelection(declaration).Contains(selection.Selection))
            {
                return declaration;
            }

            var reference =
                declaration.References.FirstOrDefault(r => r.Selection.Contains(selection.Selection));

            if (reference != null)
            {
                return reference.Declaration;
            }
        }
        return null;
    }
}
\$\endgroup\$
  • 12
    \$\begingroup\$ By reading the title in the mobile app, I thought that was a Code golf challenge \$\endgroup\$ – rpax Dec 15 '15 at 19:54
  • 2
    \$\begingroup\$ asked today; viewed 1910 times; active today Man I can't even get over 100 views. \$\endgroup\$ – 13aal Dec 16 '15 at 12:08
  • 1
    \$\begingroup\$ @Ekult3k titles, titles, titles :-) \$\endgroup\$ – Mathieu Guindon Dec 16 '15 at 12:55
  • \$\begingroup\$ @Mat'sMug Lol I was just debating about that with someone. \$\endgroup\$ – 13aal Dec 16 '15 at 13:09
11
\$\begingroup\$
public IntroduceField(RubberduckParserState parseResult, IActiveCodePaneEditor editor, IMessageBox messageBox)
{
    _declarations = parseResult.AllDeclarations.ToList();
    _editor = editor;
    _messageBox = messageBox;
}  

why don't you pass an IEnumerable<Declaration> instead of a RubberduckParserState object here ? Right now the IntroduceField(which by the way looks strange for a class name) is coupled to that object and Declaration.


Misusing of var

In the RemoveVariable() method I can't see any correct usage of the var type. var should be used only if it is obvious from the right hand side of the assignment what the type should be. In other methods this is happening too, but not that much.


RemoveExtraComma()

This method looks strange. Lets check what it is doing:

  1. if there is only one comma it will remove that comma and return the result
  2. doing a strange loop over the characters of the string and remove the first occurance of the comma and return the result
  3. if no comma is found then return the original parameter

If this ins't a bug then this can be simplified to

private static string RemoveExtraComma(string str)
{
    int commaPosition = str.IndexOf(',');
    if (commaPosition >= 0)
    {
        return str.Remove(commaPosition, 1);
    }

    return str;
}
\$\endgroup\$
  • \$\begingroup\$ Oh yeah. Class name could end with Refactoring :-) \$\endgroup\$ – Mathieu Guindon Dec 16 '15 at 12:51
  • \$\begingroup\$ RemoveExtraComma() involves more than that. If there is only one comma, it removes it. If there are multiple commas, it removes the first one without any non-whitespace characters between it and the previous comma. If there are none, it removes the last comma (I should switch to LINQ for this case too, I guess). \$\endgroup\$ – Hosch250 Dec 16 '15 at 15:19
  • \$\begingroup\$ This isn't happening with the current implementation. Do some tests to proof it. \$\endgroup\$ – Heslacher Dec 16 '15 at 15:20
17
\$\begingroup\$
public void Refactor()
{
    if (_targetDeclaration == null)
    {
        _messageBox.Show(RubberduckUI.PromoteVariable_InvalidSelection);
        return;
    }

    RemoveVariable();
    AddField();
}

Not sure if anyone other than Rubberduck's architect could point this one out, but it doesn't quite feel right. The parameterless overload of void Refactor() is intended to be the "smart" overload that figures everything out.

Your implementation assumes there's already a _targetDeclaration when that method is called, but it should really be figuring it out from the current code pane selection - and then work out whether or not to show that "invalid selection" message.


private Declaration _targetDeclaration;

The only field that's not readonly, really sticks out and makes me wonder if there wouldn't be a way to avoid having that field at all - and there is, if you make the parameterless overload work differently.

I mean, reverse the workflow - have the parameterless one figure out the target and pass it to the overload that takes it as a parameter - and then have that overload call RemoveVariable and AddField.


public void Refactor(Declaration target)
{
    if (target.DeclarationType != DeclarationType.Variable)
    {
        throw new ArgumentException("Invalid declaration type");
    }

I think an exception is fine here; this overload will be called by Rubberduck code, and this being called with any non-variable target would be a programming error - throwing an exception is the right thing to do then.


There's temporal coupling with RemoveVariable and AddField: if AddField is called first, RemoveVariable breaks because the lines in the code module are now offset. I don't really see a way to avoid it, other than making it explicit:

private void PromoteVariable(Declaration target)
{
    // must remove variable BEFORE adding the field
    RemoveVariable(target);
    AddField(target);
}

Notice I'm passing the target as a parameter instead of keeping it at instance-level.


One last thing: the RubberduckParserState parseResult constructor parameter isn't really a "parse result" anymore (vs. older versions of the code base, where the parser returned an actual result) - a better name for it could be parserState, or simply state.

\$\endgroup\$
10
\$\begingroup\$

You don't check your entry parameters for null.

In FindSelection, this query could be reduced :

var target = _declarations
    .Where(item => !item.IsBuiltIn)
    .FirstOrDefault(item => item.IsSelected(selection) && item.DeclarationType == DeclarationType.Variable
                         || item.References.Any(r => r.IsSelected(selection) &&
                            r.Declaration.DeclarationType == DeclarationType.Variable));

Since Where and FirstOrDefault use a predicate, you can merge them :

var target = _declarations
    .FirstOrDefault(item => !item.IsBuiltIn && item.IsSelected(selection) && item.DeclarationType == DeclarationType.Variable
                         || item.References.Any(r => r.IsSelected(selection) &&
                            r.Declaration.DeclarationType == DeclarationType.Variable));

Also, maybe that's just my preference, but I like to see parenthesis when there are boolean expressions that contain more than one operator (In this case && and ||). This way everything's clear concerning the priority of operations.

There are lots of long conditions in your code. If you can, you should extract methods for them, it's always clearer to read if(FooHasBar(a,b)) rather than

if(a != b.c && b is IFooBar || String.IsNullOrEmpty(a.f))
\$\endgroup\$
10
\$\begingroup\$

Exceptions

throw new ArgumentException("Invalid declaration type");

Even though there is only one parameter in this method, I should provide the argument name so the person debugging doesn't have to look. This is done in .NET 4.5, which I am using, as:

throw new ArgumentException("Invalid declaration type", "target");

For reference, C# 6.0 has a nameof() expression, which is called as nameof(target), and returns the name. This is preferable because it references the variable directly, forcing it to be updated if the variable name is ever changed.

Linq Simplification

Every time I query the _declarations list, I check that the only declarations returned are not built in (!d.IsBuiltIn) and have the type DeclarationType.Variable. I can improve readability and performance by selecting these in the constructor as (note that I changed the name as Mat's Mug suggested):

_declarations =
    parserState.AllDeclarations.Where(i => !i.IsBuiltIn && i.DeclarationType == DeclarationType.Variable)
        .ToList();
\$\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.