You are Promoted, You are Promoted, We All are Promoted!​

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 _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.Compone

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.

StackExchange Code Review Q#114050, answer score: 17