Removing parameters refactoring

Question

After the Reorder Parameters refactoring, I implemented a Remove Parameters refactoring (no, it didn't take this long, just been busy). Also, we completely refactored all the refactorings and implemented a couple interfaces for them all, which you can find here; for reference, the Remove refactoring code is found here, and the view is here.

Namespace for all these classes are Rubberduck.Refactorings.RemoveParameters.

This is the model (RemoveParametersModel):

private readonly VBProjectParseResult _parseResult;
public VBProjectParseResult ParseResult { get { return _parseResult; } }

private readonly Declarations _declarations;
public Declarations Declarations { get { return _declarations; } }

public Declaration TargetDeclaration { get; private set; }
public List<Parameter> Parameters { get; set; }

public RemoveParametersModel(VBProjectParseResult parseResult, QualifiedSelection selection)
{
    _parseResult = parseResult;
    _declarations = parseResult.Declarations;

    AcquireTarget(selection);

    Parameters = new List<Parameter>();
    LoadParameters();
}

private void AcquireTarget(QualifiedSelection selection)
{
    TargetDeclaration = FindTarget(selection, ValidDeclarationTypes);
    TargetDeclaration = PromptIfTargetImplementsInterface();
    TargetDeclaration = GetGetter();
}

private void LoadParameters()
{
    Parameters.Clear();

    var index = 0;
    Parameters = GetParameters(TargetDeclaration).Select(arg => new Parameter(arg, index++)).ToList();
}

private IEnumerable<Declaration> GetParameters(Declaration method)
{
    return Declarations.Items
                      .Where(d => d.DeclarationType == DeclarationType.Parameter
                               && d.ComponentName == method.ComponentName
                               && d.Project.Equals(method.Project)
                               && method.Context.Start.Line <= d.Selection.StartLine
                               && method.Context.Stop.Line >= d.Selection.EndLine
                               && !(method.Context.Start.Column > d.Selection.StartColumn && method.Context.Start.Line == d.Selection.StartLine)
                               && !(method.Context.Stop.Column < d.Selection.EndColumn && method.Context.Stop.Line == d.Selection.EndLine))
                      .OrderBy(item => item.Selection.StartLine)
                      .ThenBy(item => item.Selection.StartColumn);
}

public static readonly DeclarationType[] ValidDeclarationTypes =
{
    DeclarationType.Event,
    DeclarationType.Function,
    DeclarationType.Procedure,
    DeclarationType.PropertyGet,
    DeclarationType.PropertyLet,
    DeclarationType.PropertySet
};

public Declaration FindTarget(QualifiedSelection selection, DeclarationType[] validDeclarationTypes)
{
    var target = Declarations.Items
        .Where(item => !item.IsBuiltIn)
        .FirstOrDefault(item => IsSelectedDeclaration(selection, item)
                                || IsSelectedReference(selection, item));

    if (target != null && validDeclarationTypes.Contains(target.DeclarationType))
    {
        return target;
    }

    target = null;

    var targets = Declarations.Items
        .Where(item => !item.IsBuiltIn
                       && item.ComponentName == selection.QualifiedName.ComponentName
                       && validDeclarationTypes.Contains(item.DeclarationType));

    var currentSelection = new Selection(0, 0, int.MaxValue, int.MaxValue);

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

        if (currentSelection.Contains(declarationSelection) &&
            declarationSelection.Contains(selection.Selection))
        {
            target = declaration;
            currentSelection = declarationSelection;
        }

        foreach (var reference in declaration.References)
        {
            var proc = (dynamic) reference.Context.Parent;
            VBAParser.ArgsCallContext paramList;

            // This is to prevent throws when this statement fails:
            // (VBAParser.ArgsCallContext)proc.argsCall();
            try
            {
                paramList = (VBAParser.ArgsCallContext) proc.argsCall();
            }
            catch
            {
                continue;
            }

            if (paramList == null)
            {
                continue;
            }

            var referenceSelection = new Selection(paramList.Start.Line,
                paramList.Start.Column,
                paramList.Stop.Line,
                paramList.Stop.Column + paramList.Stop.Text.Length + 1);

            if (currentSelection.Contains(declarationSelection) &&
                referenceSelection.Contains(selection.Selection))
            {
                target = reference.Declaration;
                currentSelection = referenceSelection;
            }
        }
    }
    return target;
}

private Declaration PromptIfTargetImplementsInterface()
{
    var declaration = TargetDeclaration;
    var interfaceImplementation = Declarations.FindInterfaceImplementationMembers().SingleOrDefault(m => m.Equals(declaration));
    if (declaration == null || interfaceImplementation == null)
    {
        return declaration;
    }

    var interfaceMember = Declarations.FindInterfaceMember(interfaceImplementation);
    var message = string.Format(RubberduckUI.Refactoring_TargetIsInterfaceMemberImplementation, declaration.IdentifierName, interfaceMember.ComponentName, interfaceMember.IdentifierName);

    var confirm = MessageBox.Show(message, RubberduckUI.ReorderParamsDialog_TitleText, MessageBoxButtons.YesNo, MessageBoxIcon.Exclamation);
    return confirm == DialogResult.No ? null : interfaceMember;
}

private bool IsSelectedReference(QualifiedSelection selection, Declaration declaration)
{
    return declaration.References.Any(r =>
        r.QualifiedModuleName == selection.QualifiedName &&
        r.Selection.ContainsFirstCharacter(selection.Selection));
}

private bool IsSelectedDeclaration(QualifiedSelection selection, Declaration declaration)
{
    return declaration.QualifiedName.QualifiedModuleName == selection.QualifiedName
           && (declaration.Selection.ContainsFirstCharacter(selection.Selection));
}

private Declaration GetGetter()
{
    if (TargetDeclaration.DeclarationType != DeclarationType.PropertyLet &&
        TargetDeclaration.DeclarationType != DeclarationType.PropertySet)
    {
        return TargetDeclaration;
    }

    var getter = _declarations.Items.FirstOrDefault(item => item.Scope == TargetDeclaration.Scope &&
                                  item.IdentifierName == TargetDeclaration.IdentifierName &&
                                  item.DeclarationType == DeclarationType.PropertyGet);

    return getter ?? TargetDeclaration;
}

This is the actual refactoring (RemoveParametersRefactoring):

private readonly IRefactoringPresenterFactory<RemoveParametersPresenter> _factory;
private RemoveParametersModel _model;

public RemoveParametersRefactoring(IRefactoringPresenterFactory<RemoveParametersPresenter> factory)
{
    _factory = factory;
}

public void Refactor()
{
    var presenter = _factory.Create();
    if (presenter == null)
    {
        return;
    }

    _model = presenter.Show();
    if (_model == null || !_model.Parameters.Any(item => item.IsRemoved))
    {
        return;
    }

    RemoveParameters();
}

public void Refactor(QualifiedSelection target)
{
    target.Select();
    Refactor();
}

public void Refactor(Declaration target)
{
    if (!RemoveParametersModel.ValidDeclarationTypes.Contains(target.DeclarationType) && target.DeclarationType != DeclarationType.Parameter)
    {
        throw new ArgumentException("Invalid declaration type");
    }

    target.QualifiedSelection.Select();
    Refactor();
}

public void QuickFix(VBProjectParseResult parseResult, QualifiedSelection selection)
{
    _model = new RemoveParametersModel(parseResult, selection);
    var target = _model.FindTarget(selection, new[] { DeclarationType.Parameter });

    // ReSharper disable once PossibleUnintendedReferenceComparison
    _model.Parameters.Find(param => param.Declaration == target).IsRemoved = true;
    RemoveParameters();
}

private void RemoveParameters()
{
    if (_model.TargetDeclaration == null) { throw new NullReferenceException("Parameter is null."); }

    AdjustReferences(_model.TargetDeclaration.References.OrderByDescending(item => item.Selection.StartLine), _model.TargetDeclaration);
    AdjustSignatures();
}

private void AdjustReferences(IEnumerable<IdentifierReference> references, Declaration method)
{
    foreach (var reference in references.Where(item => item.Context != method.Context))
    {
        var proc = (dynamic)reference.Context.Parent;
        var module = reference.QualifiedModuleName.Component.CodeModule;
        VBAParser.ArgsCallContext paramList;

        // This is to prevent throws when this statement fails:
        // (VBAParser.ArgsCallContext)proc.argsCall();
        try { paramList = (VBAParser.ArgsCallContext)proc.argsCall(); }
        catch { continue; }

        if (paramList == null) { continue; }

        RemoveCallParameter(paramList, module);
    }
}

private void RemoveCallParameter(VBAParser.ArgsCallContext paramList, CodeModule module)
{
    var paramNames = paramList.argCall().Select(arg => arg.GetText()).ToList();
    var lineCount = paramList.Stop.Line - paramList.Start.Line + 1; // adjust for total line count

    var newContent = module.Lines[paramList.Start.Line, lineCount].Replace(" _", "").RemoveExtraSpaces();
    var currentStringIndex = 0;

    foreach (
        var param in
            _model.Parameters.Where(item => item.IsRemoved && item.Index < paramNames.Count)
                .Select(item => item.Declaration))
    {
        var paramIndex = _model.Parameters.FindIndex(item => item.Declaration.Context.GetText() == param.Context.GetText()); 
        if (paramIndex >= paramNames.Count) { return; }

        do
        {
            var paramToRemoveName = paramNames.ElementAt(0).Contains(":=")
                ? paramNames.Find(item => item.Contains(param.IdentifierName + ":="))
                : paramNames.ElementAt(paramIndex);

            if (paramToRemoveName == null || !newContent.Contains(paramToRemoveName))
            {
                continue;
            }

            var valueToRemove = paramToRemoveName != paramNames.Last()
                ? paramToRemoveName + ","
                : paramToRemoveName;

            var parameterStringIndex = newContent.IndexOf(valueToRemove, currentStringIndex, StringComparison.Ordinal);
            if (parameterStringIndex <= -1) { continue; }

            newContent = newContent.Remove(parameterStringIndex, valueToRemove.Length);

            currentStringIndex = parameterStringIndex;

            if (paramToRemoveName == paramNames.Last() && newContent.LastIndexOf(',') != -1)
            {
                newContent = newContent.Remove(newContent.LastIndexOf(','), 1);
            }
        } while (paramIndex >= _model.Parameters.Count - 1 && ++paramIndex < paramNames.Count &&
                 newContent.Contains(paramNames.ElementAt(paramIndex)));
    }

    module.ReplaceLine(paramList.Start.Line, newContent);
    module.DeleteLines(paramList.Start.Line + 1, lineCount - 1);
}

private string GetOldSignature(Declaration target)
{
    var targetModule = _model.ParseResult.ComponentParseResults.SingleOrDefault(m => m.QualifiedName == target.QualifiedName.QualifiedModuleName);
    if (targetModule == null)
    {
        return null;
    }

    var rewriter = targetModule.GetRewriter();

    var context = target.Context;
    var firstTokenIndex = context.Start.TokenIndex;
    var lastTokenIndex = -1; // will blow up if this code runs for any context other than below

    var subStmtContext = context as VBAParser.SubStmtContext;
    if (subStmtContext != null)
    {
        lastTokenIndex = subStmtContext.argList().RPAREN().Symbol.TokenIndex;
    }

    var functionStmtContext = context as VBAParser.FunctionStmtContext;
    if (functionStmtContext != null)
    {
        lastTokenIndex = functionStmtContext.asTypeClause() != null
            ? functionStmtContext.asTypeClause().Stop.TokenIndex
            : functionStmtContext.argList().RPAREN().Symbol.TokenIndex;
    }

    var propertyGetStmtContext = context as VBAParser.PropertyGetStmtContext;
    if (propertyGetStmtContext != null)
    {
        lastTokenIndex = propertyGetStmtContext.asTypeClause() != null
            ? propertyGetStmtContext.asTypeClause().Stop.TokenIndex
            : propertyGetStmtContext.argList().RPAREN().Symbol.TokenIndex;
    }

    var propertyLetStmtContext = context as VBAParser.PropertyLetStmtContext;
    if (propertyLetStmtContext != null)
    {
        lastTokenIndex = propertyLetStmtContext.argList().RPAREN().Symbol.TokenIndex;
    }

    var propertySetStmtContext = context as VBAParser.PropertySetStmtContext;
    if (propertySetStmtContext != null)
    {
        lastTokenIndex = propertySetStmtContext.argList().RPAREN().Symbol.TokenIndex;
    }

    var declareStmtContext = context as VBAParser.DeclareStmtContext;
    if (declareStmtContext != null)
    {
        lastTokenIndex = declareStmtContext.STRINGLITERAL().Last().Symbol.TokenIndex;
        if (declareStmtContext.argList() != null)
        {
            lastTokenIndex = declareStmtContext.argList().RPAREN().Symbol.TokenIndex;
        }
        if (declareStmtContext.asTypeClause() != null)
        {
            lastTokenIndex = declareStmtContext.asTypeClause().Stop.TokenIndex;
        }
    }

    var eventStmtContext = context as VBAParser.EventStmtContext;
    if (eventStmtContext != null)
    {
        lastTokenIndex = eventStmtContext.argList().RPAREN().Symbol.TokenIndex;
    }

    return rewriter.GetText(new Interval(firstTokenIndex, lastTokenIndex));
}

private string ReplaceCommas(string signature, int indexParamRemoved)
{
    if (signature.Count(c => c == ',') > indexParamRemoved) { indexParamRemoved++; }

    for (int i = 0, commaCounter = 0; i < signature.Length && indexParamRemoved != 0; i++)
    {
        if (signature.ElementAt(i) == ',')
        {
            commaCounter++;
        }

        if (commaCounter == indexParamRemoved)
        {
            return signature.Remove(i, 1);
        }
    }

    return signature;
}

private void AdjustSignatures()
{
    var proc = (dynamic)_model.TargetDeclaration.Context;
    var paramList = (VBAParser.ArgListContext)proc.argList();
    var module = _model.TargetDeclaration.QualifiedName.QualifiedModuleName.Component.CodeModule;

    // if we are adjusting a property getter, check if we need to adjust the letter/setter too
    if (_model.TargetDeclaration.DeclarationType == DeclarationType.PropertyGet)
    {
        var setter = GetLetterOrSetter(_model.TargetDeclaration, DeclarationType.PropertySet);
        if (setter != null)
        {
            AdjustSignatures(setter);
        }

        var letter = GetLetterOrSetter(_model.TargetDeclaration, DeclarationType.PropertyLet);
        if (letter != null)
        {
            AdjustSignatures(letter);
        }
    }

    RemoveSignatureParameters(_model.TargetDeclaration, paramList, module);

    foreach (var withEvents in _model.Declarations.Items.Where(item => item.IsWithEvents && item.AsTypeName == _model.TargetDeclaration.ComponentName))
    {
        foreach (var reference in _model.Declarations.FindEventProcedures(withEvents))
        {
            AdjustReferences(reference.References.OrderByDescending(item => item.Selection.StartLine), reference);
            AdjustSignatures(reference);
        }
    }

    var interfaceImplementations = _model.Declarations.FindInterfaceImplementationMembers()
                                                .Where(item => item.Project.Equals(_model.TargetDeclaration.Project) &&
                                                       item.IdentifierName == _model.TargetDeclaration.ComponentName + "_" + _model.TargetDeclaration.IdentifierName);
    foreach (var interfaceImplentation in interfaceImplementations)
    {
        AdjustReferences(interfaceImplentation.References.OrderByDescending(item => item.Selection.StartLine), interfaceImplentation);
        AdjustSignatures(interfaceImplentation);
    }
}

private Declaration GetLetterOrSetter(Declaration declaration, DeclarationType declarationType)
{
    return _model.Declarations.Items.FirstOrDefault(item => item.Scope == declaration.Scope &&
                      item.IdentifierName == declaration.IdentifierName &&
                      item.DeclarationType == declarationType);
}

private void AdjustSignatures(Declaration declaration)
{
    var proc = (dynamic)declaration.Context.Parent;
    var module = declaration.QualifiedName.QualifiedModuleName.Component.CodeModule;
    VBAParser.ArgListContext paramList;

    if (declaration.DeclarationType == DeclarationType.PropertySet ||
        declaration.DeclarationType == DeclarationType.PropertyLet)
    {
        paramList = (VBAParser.ArgListContext)proc.children[0].argList();
    }
    else
    {
        paramList = (VBAParser.ArgListContext)proc.subStmt().argList();
    }

    RemoveSignatureParameters(declaration, paramList, module);
}

private void RemoveSignatureParameters(Declaration target, VBAParser.ArgListContext paramList, CodeModule module)
{
    var paramNames = paramList.arg();

    var paramsRemoved = _model.Parameters.Where(item => item.IsRemoved).ToList();
    var signature = GetOldSignature(target);

    foreach (var param in paramsRemoved)
    {
        try
        {
            signature = ReplaceCommas(signature.Replace(paramNames.ElementAt(param.Index).GetText(), ""), _model.Parameters.FindIndex(item => item == param) - paramsRemoved.FindIndex(item => item == param));
        }
        catch (ArgumentOutOfRangeException)
        {
        }
    }
    var lineNum = paramList.GetSelection().LineCount;

    module.ReplaceLine(paramList.Start.Line, signature);
    module.DeleteLines(paramList.Start.Line + 1, lineNum - 1);
}

And this is the presenter (RemoveParametersPresenter):

private readonly IRemoveParametersView _view;
private readonly RemoveParametersModel _model;

public RemoveParametersPresenter(IRemoveParametersView view, RemoveParametersModel model)
{
    _view = view;
    _model = model;
}

public RemoveParametersModel Show()
{
    if (_model.Parameters.Count == 0)
    {
        var message = string.Format(RubberduckUI.RemovePresenter_NoParametersError, _model.TargetDeclaration.IdentifierName);
        MessageBox.Show(message, RubberduckUI.RemoveParamsDialog_TitleText, MessageBoxButtons.OK, MessageBoxIcon.Exclamation);
        return null;
    }

    if (_model.Parameters.Count == 1)
    {
        _model.Parameters[0].IsRemoved = true;
        return _model;
    }

    _view.Parameters = _model.Parameters;
    _view.InitializeParameterGrid();

    if (_view.ShowDialog() != DialogResult.OK)
    {
        return null;
    }

    _model.Parameters = _view.Parameters;
    return _model;
}

As always, I welcome all comments.

Answer 1

That's quite a lot to review. And it looks mostly pretty nice. I only have some pretty minor nitpicks.

---

    if (declareStmtContext.argList() != null)
    {
        lastTokenIndex = declareStmtContext.argList().RPAREN().Symbol.TokenIndex;
    }
    if (declareStmtContext.asTypeClause() != null)
    {
        lastTokenIndex = declareStmtContext.asTypeClause().Stop.TokenIndex;
    }

I'm not a big fan of reassigning values. If the second condition is true, lastTokenIndex will be overwritten, and it will have been pointless. This should be equivalent, with no reassignment and no unnecessary statements:

    if (declareStmtContext.asTypeClause() != null)
    {
        lastTokenIndex = declareStmtContext.asTypeClause().Stop.TokenIndex;
    }
    else if (declareStmtContext.argList() != null)
    {
        lastTokenIndex = declareStmtContext.argList().RPAREN().Symbol.TokenIndex;
    }

That is, I just moved the second condition up, and chained the two conditions with else if.

---

When the statement in the foreach is so long as here:

foreach (
    var param in
        _model.Parameters.Where(item => item.IsRemoved && item.Index < paramNames.Count)
            .Select(item => item.Declaration))
{

I'd recommend to extract the _model.Parameters.Where... part into a local variable before the loop, like this:

var removedDeclarations =
    _model.Parameters
    .Where(item => item.IsRemoved && item.Index < paramNames.Count)
    .Select(item => item.Declaration)

foreach (var param in removedDeclarations) { ... }

Note that this has the drawback that it exposes removedDeclarations available outside the scope of the foreach. So there's a trade-off, I let you decide the lesser evil.

Another alternative is to extract the query to a method. That way you could avoid exposing a local variable outside the foreach, something along the lines of:

foreach (var param in GetRemovedDeclarations(...)) { ... }

---

Some boolean conditions are really complex, for example:

return Declarations.Items
                  .Where(d => d.DeclarationType == DeclarationType.Parameter
                           && d.ComponentName == method.ComponentName
                           && d.Project.Equals(method.Project)
                           && method.Context.Start.Line <= d.Selection.StartLine
                           && method.Context.Stop.Line >= d.Selection.EndLine
                           && !(method.Context.Start.Column > d.Selection.StartColumn && method.Context.Start.Line ==  d.Selection.StartLine)
                           && !(method.Context.Stop.Column < d.Selection.EndColumn && method.Context.Stop.Line == d.Selection.EndLine))

It might be a good idea to extract to a function with a descriptive name.