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

Removing parameters refactoring

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

Problem

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 Parameters { get; set; }

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

AcquireTarget(selection);

Parameters = new List();
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 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.EndLine
&& !(method.Context.Start.Column > d.Selection.StartColumn && method.Context.Start.Line == d.Sel

Solution

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

Code Snippets

if (declareStmtContext.argList() != null)
    {
        lastTokenIndex = declareStmtContext.argList().RPAREN().Symbol.TokenIndex;
    }
    if (declareStmtContext.asTypeClause() != null)
    {
        lastTokenIndex = declareStmtContext.asTypeClause().Stop.TokenIndex;
    }
if (declareStmtContext.asTypeClause() != null)
    {
        lastTokenIndex = declareStmtContext.asTypeClause().Stop.TokenIndex;
    }
    else if (declareStmtContext.argList() != null)
    {
        lastTokenIndex = declareStmtContext.argList().RPAREN().Symbol.TokenIndex;
    }
foreach (
    var param in
        _model.Parameters.Where(item => item.IsRemoved && item.Index < paramNames.Count)
            .Select(item => item.Declaration))
{
var removedDeclarations =
    _model.Parameters
    .Where(item => item.IsRemoved && item.Index < paramNames.Count)
    .Select(item => item.Declaration)

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

Context

StackExchange Code Review Q#93595, answer score: 5

Revisions (0)

No revisions yet.