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;    }}
You are Promoted, You are Promoted, We All are Promoted!
c#;meta programming;rubberduck
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:  if there is only one comma it will remove that comma and return the resultdoing a strange loop over the characters of the string and remove the first occurance of the comma and return the result  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;}