Climbing the Totem Pole - Local Variables Become Parameters

Along with the Introduce Field refactoring, I wrote an Introduce Parameter refactoring.

I actually wrote this refactoring first, but due to a bug in the parser/resolver, I was unable to fully test it until afterward. Much of this codebase is shared, and I am going to write a helper class to support these, and other, refactorings, but other than this, this looks ready to go to me. Please point out anything that you are concerned about.

```
public class IntroduceParameter : IRefactoring
{
private readonly RubberduckParserState _parseResult;
private readonly IList _declarations;
private readonly IActiveCodePaneEditor _editor;
private readonly IMessageBox _messageBox;

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

public IntroduceParameter(RubberduckParserState parseResult, IActiveCodePaneEditor editor, IMessageBox messageBox)
{
_parseResult = parseResult;
_declarations = parseResult.AllDeclarations.ToList();
_editor = editor;
_messageBox = messageBox;
}

public void Refactor()
{
var selection = _editor.GetSelection();

if (!selection.HasValue)
{
return;
}

Refactor(selection.Value);
}

public void Refactor(QualifiedSelection selection)
{
var target = FindSelection(selection);

PromoteVariable(target);
}

public void Refactor(Declaration target)
{
PromoteVariable(target);
}

private void PromoteVariable(Declaration target)
{
if (target == null || target.DeclarationType != DeclarationType.Variable)
{
_messageBox.Show(RubberduckUI.PromoteVariable_InvalidSelection,
RubberduckUI.IntroduceParameter_TitleText, MessageBoxButtons.OK, Message

I would not complain if you choose to keep the code as it is. It's readable and focused, except for the helper methods which you have already identified.

But we know that code is never finished, and there are always opportunities for improvements. So here are my nitpicks:

In Refactor(QualifiedSelection)

You have a parameter selection, yet you pass that to FindSelection(). Do you have a selection or not? If you have one, why do you need to find it? If you need to find it, then it isn't really a selection, is it?

I don't want all these questions when I read code. My mind gets overloaded pretty quickly. And it shouldn't be necessary to look into all the members just to verify if they do what they say.

FindSelection() returns a Declaration. I think FindDeclaration() would be a better name.

In PromoteVariable()

RemoveVariable() is called before UpdateSignature(), yet I have to scroll pretty far down to find RemoveVariable() while UpdateSignature() is the very first I see.

I would like to see RemoveVariable() listed first.

IMessageBox

I like this interface, but the only variation, except from the text, are the buttons. Why not abstract the usage instead of copying the actual interface?

Like:

public interface IMessageBox
{
    void Inform(string message);
    bool Confirm(string question);
}

In RemoveExtraComma()

It's hard to see if the logic is correct:

• Is it correct to simply remove a comma if there is only one?

• How many commas can there be? Is it possible that the method returns too early and remove the wrong comma?

• Will a reorder of the if statements change anything?

• The if conditions are complex. I would like to see variables or methods which explains what we are looking for, instead of having to parse them each time.

The statement:

statement.children.Count(i => i is VBAParser.VariableSubStmtContext) > 1

Can be written as:

statement.children.OfType().Count() > 1

GetParameterDefinition()

Could be renamed to CreateParameterDefinition() or DefineParameterDefinition(). You are returning something that doesn't exist in the target code, so ,in my opinion, it shouldn't be a Get...().

HasMultipleDeclarationsInStatement()

Can be moved to Declaration.

Wrap lists in types

The field

IList _declarations

Could be wrapped inside of a DeclarationList or DeclarationCollection and the related methods could be moved over there.

StackExchange Code Review Q#114466, answer score: 2