Rubberduck's "Rename" refactoring implementation

Question

Knowing who's using what, and where, I've implemented a "Rename" refactoring for Rubberduck.

It works great - it needs further extensive testing, but the preliminary tests are very, very exciting.

There are a few things I'm not sure I like though:

• The entire logic is implemented in the Rubberduck.UI namespace. The "Extract Method" refactoring logic is also implemented under that namespace (under Rubberduck.UI.Refactorings.ExtractMethod) - I think I might be violating SRP with these presenter classes, but I'm not sure it's worth the trouble. Any thoughts?
• I don't know whether/how it's possible to actually replace tokens in the parse tree, so the renaming actually boils down to a very very localized search & replace... and the implementation is ugly beyond words - and I'm not sure how to go about making it right.
• The code acquiring the target identifier looks like it could get cleaner... but how?

namespace Rubberduck.UI.Refactorings.Rename
{
    public class RenamePresenter
    {
        private readonly VBE _vbe;
        private readonly IRenameView _view;
        private readonly Declarations _declarations;
        private readonly QualifiedSelection _selection;

        public RenamePresenter(VBE vbe, IRenameView view, Declarations declarations, QualifiedSelection selection)
        {
            _vbe = vbe;
            _view = view;
            _view.OkButtonClicked += OnOkButtonClicked;

            _declarations = declarations;
            _selection = selection;
        }

        public void Show()
        {
            AcquireTarget(_selection);
            _view.ShowDialog();
        }

        private static readonly DeclarationType[] ModuleDeclarationTypes =
            {
                DeclarationType.Class,
                DeclarationType.Module
            };

        private void OnOkButtonClicked(object sender, EventArgs e)
        {
            if (ModuleDeclarationTypes.Contains(_view.Target.DeclarationType))
            {
                RenameModule();
            }
            else
            {
                RenameDeclaration();
            }

            RenameUsages();
        }

        private void RenameModule()
        {
            try
            {
                var module = _vbe.FindCodeModules(_view.Target.QualifiedName.QualifiedModuleName).Single();
                module.Name = _view.NewName;
            }
            catch (COMException exception)
            {
                MessageBox.Show(RubberduckUI.RenameDialog_ModuleRenameError, RubberduckUI.RenameDialog_Caption);
            }
        }

        private void RenameDeclaration()
        {
            var module = _vbe.FindCodeModules(_view.Target.QualifiedName.QualifiedModuleName).First();
            var content = module.get_Lines(_view.Target.Selection.StartLine, 1);
            var newContent = GetReplacementLine(content, _view.Target.IdentifierName, _view.NewName);
            module.ReplaceLine(_view.Target.Selection.StartLine, newContent);
        }

        private void RenameUsages()
        {
            var modules = _view.Target.References.GroupBy(r => r.QualifiedModuleName);
            foreach (var grouping in modules)
            {
                var module = _vbe.FindCodeModules(grouping.Key).First();
                foreach (var line in grouping.GroupBy(reference => reference.Selection.StartLine))
                {
                    var content = module.get_Lines(line.Key, 1);
                    var newContent = GetReplacementLine(content, _view.Target.IdentifierName, _view.NewName);
                    module.ReplaceLine(line.Key, newContent);
                }
            }
        }

        private string GetReplacementLine(string content, string target, string newName)
        {
            // until we figure out how to replace actual tokens,
            // this is going to have to be done the ugly way...

            // what we're trying to avoid here,
            // is to replace all instances of "Foo" in "Foo = FooBar" when target is "Foo".

            var result = ' ' + content;
            if (result.Contains(' ' + target))
            {
                result = result.Replace(' ' + target, ' ' + newName);
            }
            if (result.Contains(target + ' '))
            {
                result = result.Replace(target + ' ', newName + ' ');
            }
            if (result.Contains(target + '.'))
            {
                result = result.Replace(target + '.', newName + '.');
            }
            else if (result.Contains('.' + target))
            {
                result = result.Replace('.' + target, '.'+ newName);
            }

            if (result.Contains('(' + target))
            {
                result = result.Replace('(' + target, '(' + newName);
            }

            if (result.Contains(":=" + target))
            {
                result = result.Replace(":=" + target, ":=" + newName);
            }

            if (result.Contains(target + '!'))
            {
                result = result.Replace(target + '!', newName + '!');
            }
            else if (result.Contains('!' + target))
            {
                result = result.Replace('!' + target, '!' + newName);
            }

            return result.Substring(1);
        }

        private static readonly DeclarationType[] ProcedureDeclarationTypes =
            {
                DeclarationType.Procedure,
                DeclarationType.Function,
                DeclarationType.PropertyGet,
                DeclarationType.PropertyLet,
                DeclarationType.PropertySet
            };

        private void AcquireTarget(QualifiedSelection selection)
        {
            var targets = _declarations.Items.Where(declaration =>
                declaration.QualifiedName.QualifiedModuleName == selection.QualifiedName
                && (declaration.Selection.Contains(selection.Selection))
                || declaration.References.Any(r => r.Selection.Contains(selection.Selection)))
                .ToList();

            var nonProcTarget = targets.Where(t => !ProcedureDeclarationTypes.Contains(t.DeclarationType)).ToList();
            if (nonProcTarget.Any())
            {
                _view.Target = nonProcTarget.First();
            }
            else
            {
                _view.Target = targets.FirstOrDefault();
            }

            if (_view.Target == null)
            {
                // no valid selection? no problem - let's rename the module:
                _view.Target = _declarations.Items.SingleOrDefault(declaration =>
                    declaration.QualifiedName.QualifiedModuleName == selection.QualifiedName
                    && ModuleDeclarationTypes.Contains(declaration.DeclarationType));
            }
        }
    }
}

I definitely need a different approach for the GetReplacementLine method; I thought of using regex, but I'd rather not. Or should I? Are there other alternatives?

I've tagged this with antlr because my Declaration object does expose a RuleContext object that I'm just not using here... anyone familiar with ANTLR knows if there's something I should know that would make my life easier here?

Answer 1

GetReplacementLine()

Instead of using String.Replace() you should check out the TokenRewriteStream mentioned in this answer.

If you need to use String.Replace() then you should omit the not needed call to String.Contains(). If the string which should be replaced isn't found in the 'content', the unchanged 'content' will be returned. In addition it will speed up the execution because it does not have to search for the token twice.

But you also have duplicated code inside this method, so it would be better to extract this into 2 separate methods. One replacing the searchterm with a passed prefix and the other with a passed postfix.

Like:

private string PostfixReplace(string content, string token, string newName, string postFix)
{
    return content.Replace(token + postFix, newName + postFix);
}

AcquireTarget()

There is no need to call ToList() on the result of the Linq Where clauses because you only need either First() or FirstOrDefault(). The call to ToList() will slow down the execution because every item will be qualified but you only need the first one.

var

IMHO you are misusing the var keyword, because you use it all the time. Assume you need to dig into this class after not touching it for a few weeks, you won't know what most of the types will be, because it isn't obvious what type the right side is, e.g

var targets = _declarations.Items.Where(declaration =>
    declaration.QualifiedName.QualifiedModuleName == selection.QualifiedName
    && (declaration.Selection.Contains(selection.Selection))
    || declaration.References.Any(r => r.Selection.Contains(selection.Selection)))
    .ToList();