10
\$\begingroup\$

One of my latest refactorings for Rubberduck is Move Closer To Usage. The refactoring will take a field and move it just above the reference to it only if it is used in a single method, or take a variable declaration and move it just above its first call.

As promised, I moved many of the duplicated support methods into extension methods, although I have a few left that are duplicated between a few refactorings. Also, thanks to Thomas Eyde, I found some bugs in RemoveComma(), which are now fixed.

Additionally, this refactoring does not have a user interface, so there is just one file. Be sure to let me know what you think!

public class MoveCloserToUsageRefactoring : IRefactoring
{
    private readonly List<Declaration> _declarations;
    private readonly IActiveCodePaneEditor _editor;
    private readonly IMessageBox _messageBox;

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

    public void Refactor()
    {
        var qualifiedSelection = _editor.GetSelection();
        if (qualifiedSelection != null)
        {
            Refactor(_declarations.FindVariable(qualifiedSelection.Value));
        }
        else
        {
            _messageBox.Show("Invalid Selection.", "Rubberduck - Move Closer To Usage", System.Windows.Forms.MessageBoxButtons.OK,
                System.Windows.Forms.MessageBoxIcon.Exclamation);
        }
    }

    public void Refactor(QualifiedSelection selection)
    {
        Refactor(_declarations.FindVariable(selection));
    }

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

        if (!target.References.Any())
        {
            var message = string.Format(RubberduckUI.MoveCloserToUsage_TargetHasNoReferences, target.IdentifierName);

            _messageBox.Show(message, RubberduckUI.MoveCloserToUsage_Caption, System.Windows.Forms.MessageBoxButtons.OK,
                System.Windows.Forms.MessageBoxIcon.Exclamation);

            return;
        }

        if (TargetIsReferencedFromMultipleMethods(target))
        {
            var message = string.Format(RubberduckUI.MoveCloserToUsage_TargetIsUsedInMultipleMethods, target.IdentifierName);
            _messageBox.Show(message, RubberduckUI.MoveCloserToUsage_Caption, System.Windows.Forms.MessageBoxButtons.OK,
                System.Windows.Forms.MessageBoxIcon.Exclamation);

            return;
        }

        MoveDeclaration(target);
    }

    private bool TargetIsReferencedFromMultipleMethods(Declaration target)
    {
        var firstReference = target.References.FirstOrDefault();

        return firstReference != null && target.References.Any(r => r.ParentScope != firstReference.ParentScope);
    }

    private void MoveDeclaration(Declaration target)
    {
        InsertDeclaration(target);
        RemoveVariable(target);
    }

    private void InsertDeclaration(Declaration target)
    {
        var firstReference = target.References.OrderBy(r => r.Selection.StartLine).First();

        var oldLines = _editor.GetLines(firstReference.Selection);
        var newLines = oldLines.Insert(firstReference.Selection.StartColumn - 1, GetDeclarationString(target));

        _editor.DeleteLines(firstReference.Selection);
        _editor.InsertLines(firstReference.Selection.StartLine, newLines);
    }

    private string GetDeclarationString(Declaration target)
    {
        return Environment.NewLine + "    Dim " + target.IdentifierName + " As " + target.AsTypeName + Environment.NewLine;
    }

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

        var variableStmtContext = target.GetVariableStmtContext();

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

        var oldLines = _editor.GetLines(selection);

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

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

        _editor.DeleteLines(selection);

        if (newLines.Trim() != string.Empty)
        {
            _editor.InsertLines(selection.StartLine, newLines);
        }
    }

    private string RemoveExtraComma(string str, int numParams, int indexRemoved)
    {
        // Example use cases for this method (fields and variables):
        // Dim fizz as Boolean, dizz as Double
        // Private fizz as Boolean, dizz as Double
        // Public fizz as Boolean, _
        //        dizz as Double
        // Private fizz as Boolean _
        //         , dizz as Double _
        //         , iizz as Integer

        // Before this method is called, the parameter to be removed has 
        // already been removed.  This means 'str' will look like:
        // Dim fizz as Boolean, 
        // Private , dizz as Double
        // Public fizz as Boolean, _
        //        
        // Private  _
        //         , dizz as Double _
        //         , iizz as Integer

        // This method is responsible for removing the redundant comma
        // and returning a string similar to:
        // Dim fizz as Boolean
        // Private dizz as Double
        // Public fizz as Boolean _
        //        
        // Private  _
        //          dizz as Double _
        //         , iizz as Integer

        var commaToRemove = numParams == indexRemoved ? indexRemoved - 1 : indexRemoved;

        return str.Remove(str.NthIndexOf(',', commaToRemove), 1);
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ It seems like RemoveExtraComma should also clean up a trailing underscore... and that mega-comment (doc?) should probably use /* ... */ comment block syntax. \$\endgroup\$ Dec 24 '15 at 20:03
  • \$\begingroup\$ @Mat'sMug Good idea. Thing is, it would be quick tricky to know which underscores to keep. This should actually be a separate inspection that I could perhaps run automatically - nicely format field/variable declarations. \$\endgroup\$
    – user34073
    Dec 24 '15 at 20:10
  • \$\begingroup\$ Well, you only ever need to remove the last underscore. Not removing it could break the code if there's anything on the next line. \$\endgroup\$ Dec 24 '15 at 20:40
  • \$\begingroup\$ Hmm, that could be a bug. However, it could also break the code if I removed the wrong one, or make it ugly at a minimum. \$\endgroup\$
    – user34073
    Dec 24 '15 at 21:20
4
\$\begingroup\$

There's not a lot to be said in my opinion. You've done a good job of using variables where they help clarify the intent and everything seems to be well named. The code is easy to read, even if some lines get kind of long and could be split across several lines. (Linq has a way of going sideways instead of down if you let it.)

There is this here.

throw new ArgumentException(@"Invalid Argument", "target")

I copied it in isolation to simulate what would happen if this gets thrown. Can you tell what exactly went wrong? No? Maybe some context would help.

    if (target.DeclarationType != DeclarationType.Variable)
    {
        throw new ArgumentException(@"Invalid Argument", "target");

Crystal clear, but we had to look at the code to know it. I'd recommend a better message.

    if (target.DeclarationType != DeclarationType.Variable)
    {
        throw new ArgumentException("Invalid Argument. DeclarationType must be a Variable", "target");

Also note that there wasn't a need for the string literal identifier (@).

There's also this.

    {
        _messageBox.Show("Invalid Selection.", "Rubberduck - Move Closer To Usage", System.Windows.Forms.MessageBoxButtons.OK,
            System.Windows.Forms.MessageBoxIcon.Exclamation);
    }
  • Those strings should be in a resource file so they can be localized. Rubberduck has been translated into several languages at this point.
  • I'm not entirely sure why you fully qualified the MessageBox enums. If you don't want to import the Forms namespace, I understand. However, you could alias the enums that you need to interact with the IMessageBox abstraction.

     using MessageBoxButtons = System.Windows.Forms.MessageBoxButtons;
     using MessageBoxIcons = System.Windows.Forms.MessageBoxIcons;
    
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Nice answer. I didn't mean to fully qualify the MessageBox enums, it just goes in like that by default, and I didn't change it. You are right about the exception parameter, I need to fix that. As for the localized strings, that was an overlook - most of the new strings have been localized. \$\endgroup\$
    – user34073
    Dec 26 '15 at 14:45
  • \$\begingroup\$ P.S. That '@' was to shut R# up without using a comment. \$\endgroup\$
    – user34073
    Dec 26 '15 at 15:57

Your Answer

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