HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Rubberduck's "Rename" refactoring implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
refactoringimplementationrubberduckrename

Problem

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))
{

Solution

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();

Code Snippets

private string PostfixReplace(string content, string token, string newName, string postFix)
{
    return content.Replace(token + postFix, newName + postFix);
}
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();

Context

StackExchange Code Review Q#85141, answer score: 5

Revisions (0)

No revisions yet.