patterncsharpMinor
Removing parameters refactoring
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
This is the model (
```
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
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.
I'm not a big fan of reassigning values.
If the second condition is true,
and it will have been pointless.
This should be equivalent,
with no reassignment and no unnecessary statements:
That is, I just moved the second condition up,
and chained the two conditions with
When the statement in the
I'd recommend to extract the
Note that this has the drawback that it exposes
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
something along the lines of:
Some boolean conditions are really complex, for example:
It might be a good idea to extract to a function with a descriptive name.
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.