Return to Question

This post is somewhat of a follow-up to Rubberduck's "Extract Method" refactoring implementation.

A tremendous amount of things have changed since version 1.2, when this feature was first introduced - the more important thing being the advent of the Declarations API, which implies that the feature now not only gets an ANTLR parse tree to play with, but also a strongly-typed IEnumerable<Declaration> collection, each with their References resolved (see this post for more details on that specific part).

This post is somewhat of a follow-up to Rubberduck's "Extract Method" refactoring implementation.

A tremendous amount of things have changed since version 1.2, when this feature was first introduced - the more important thing being the advent of the Declarations API, which implies that the feature now not only gets an ANTLR parse tree to play with, but also a strongly-typed IEnumerable<Declaration> collection, each with their References resolved (see this post for more details on that specific part).

For context, here's the ExtractedParameter class:

namespace Rubberduck.UI.Refactorings.ExtractMethod
{
 public class ExtractedParameter
 {
 public enum PassedBy
 {
 ByRef,
 ByVal
 }
 public static readonly string None = RubberduckUI.ExtractMethod_OutputNone;
 private readonly string _name;
 private readonly string _typeName;
 private readonly PassedBy _passedBy;
 public ExtractedParameter(string typeName, PassedBy passedBy, string name = null)
 {
 _name = name ?? None;
 _typeName = typeName;
 _passedBy = passedBy;
 }
 public string Name
 {
 get { return _name; }
 }
 public string TypeName
 {
 get { return _typeName; }
 }
 public override string ToString()
 {
 return _passedBy.ToString() + ' ' + Name + ' ' + Tokens.As + ' ' + TypeName;
 }
 }
}

A tremendous amount of things have changed since version 1.2, when this that feature was first introduced - the more important thing being the advent of the Declarations API, which implies that the feature now not only gets an ANTLR parse tree to play with, but also a strongly-typed IEnumerable<Declaration> collection, each with their References resolved (see this post for more details on that specific part).

namespace Rubberduck.UI.Refactorings.ExtractMethod
{
 public class ExtractMethodPresenter
 {
 private readonly IExtractMethodDialog _view;
 private readonly IEnumerable<ExtractedParameter> _input;
 private readonly IEnumerable<ExtractedParameter> _output;
 private readonly List<Declaration> _locals;
 private readonly List<Declaration> _toRemoveFromSource;
 private readonly string _selectedCode;
 private readonly QualifiedSelection _selection;
 private readonly IActiveCodePaneEditor _editor;
 private readonly Declaration _member;
 private readonly HashSet<Declaration> _usedInSelection;
 private readonly HashSet<Declaration> _usedBeforeSelection;
 private readonly HashSet<Declaration> _usedAfterSelection;
 public ExtractMethodPresenter(IActiveCodePaneEditor editor, IExtractMethodDialog view, Declaration member, QualifiedSelection selection, Declarations declarations)
 {
 _editor = editor;
 _view = view;
 _member = member;
 _selection = selection;
 _selectedCode = _editor.GetLines(selection.Selection);
 var inScopeDeclarations = declarations.Items.Where(item => item.ParentScope == member.Scope).ToList();
 var inSelection = inScopeDeclarations.SelectMany(item => item.References)
 .Where(item => selection.Selection.Contains(item.Selection))
 .ToList();
 _usedInSelection = new HashSet<Declaration>(inScopeDeclarations.Where(item =>
 item.References.Any(reference => inSelection.Contains(reference))));
 _usedBeforeSelection = new HashSet<Declaration>(inScopeDeclarations.Where(item => 
 item.References.Any(reference => reference.Selection.StartLine < selection.Selection.StartLine)));
 _usedAfterSelection = new HashSet<Declaration>(inScopeDeclarations.Where(item =>
 item.References.Any(reference => reference.Selection.StartLine > selection.Selection.EndLine)));
 // identifiers used inside selection and before selection (or if it's a parameter) are candidates for parameters:
 var input = inScopeDeclarations.Where(item => 
 _usedInSelection.Contains(item) && (_usedBeforeSelection.Contains(item) || item.DeclarationType == DeclarationType.Parameter)).ToList();
 // identifiers used inside selection and after selection are candidates for return values:
 var output = inScopeDeclarations.Where(item => 
 _usedInSelection.Contains(item) && _usedAfterSelection.Contains(item))
 .ToList();
 // identifiers used only inside and/or after selection are candidates for locals:
 _locals = inScopeDeclarations.Where(item => item.DeclarationType != DeclarationType.Parameter && (
 item.References.All(reference => inSelection.Contains(reference))
 || (_usedAfterSelection.Contains(item) && (!_usedBeforeSelection.Contains(item)))))
 .ToList();
 // locals that are only used in selection are candidates for being moved into the new method:
 _toRemoveFromSource = _locals.Where(item => !_usedAfterSelection.Contains(item)).ToList();
 _output = output.Select(declaration =>
 new ExtractedParameter(declaration.IdentifierName, declaration.AsTypeName, ExtractedParameter.PassedBy.ByRef));
 _input = input.Where(declaration => !output.Contains(declaration))
 .Select(declaration =>
 new ExtractedParameter(declaration.IdentifierName, declaration.AsTypeName, ExtractedParameter.PassedBy.ByVal));
 }
 public void Show()
 {
 _view.MethodName = "Method1";
 _view.Inputs = _input.ToList();
 _view.Outputs = _output.ToList();
 _view.Locals = _locals.Select(variable => new ExtractedParameter(variable.IdentifierName, variable.AsTypeName, ExtractedParameter.PassedBy.ByVal)).ToList();
 var returnValues = new[] {new ExtractedParameter("(none)", string.Empty, ExtractedParameter.PassedBy.ByVal)}
 .Union(_view.Outputs)
 .Union(_view.Inputs)
 .ToList();
 _view.ReturnValues = returnValues;
 _view.ReturnValue = _output.Count() == 1
 ? _output.Single()
 : returnValues.First();
 _view.RefreshPreview += _view_RefreshPreview;
 _view.OnRefreshPreview();

 var result = _view.ShowDialog();
 if (result != DialogResult.OK)
 {
 return;
  }
 _editor.DeleteLines(_selection.Selection);
 _editor.ReplaceLine(_selection.Selection.StartLine, GetMethodCall());
 var insertionLine = _member.Context.GetSelection().EndLine - _selection.Selection.LineCount + 2;
 _editor.InsertLines(insertionLine, GetExtractedMethod());
 // assumes these are declared *before* the selection...
 var offset = 0;
 foreach (var declaration in _toRemoveFromSource.OrderBy(e => e.Selection.StartLine))
 {
 var target = new Selection(
 declaration.Selection.StartLine - offset,
 declaration.Selection.StartColumn,
 declaration.Selection.EndLine - offset,
 declaration.Selection.EndColumn);
 _editor.DeleteLines(target);
 offset += declaration.Selection.LineCount;
 }
 }
 private void _view_RefreshPreview(object sender, EventArgs e)
 {
 var hasReturnValue = _view.ReturnValue != null && _view.ReturnValue.Name != "(none)";
 _view.CanSetReturnValue =
 hasReturnValue && !IsValueType(_view.ReturnValue.TypeName);
 GeneratePreview();
 }
 private void GeneratePreview()
 {
 _view.Preview = GetExtractedMethod();
 }
 private string GetMethodCall()
 {
 string result;
 var returnValueName = _view.ReturnValue.Name;
 var argsList = string.Join(", ", _view.Parameters.Select(p => p.Name));
 if (returnValueName != "(none)")
 {
 var setter = _view.SetReturnValue ? Tokens.Set + ' ' : string.Empty;
 result = setter + returnValueName + " = " + _view.MethodName + '(' + argsList + ')';
 }
 else
 {
 result = _view.MethodName + ' ' + argsList;
 }
 return " " + result; // todo: smarter indentation
 }
 private static readonly IEnumerable<string> ValueTypes = new[]
 {
 Tokens.Boolean,
 Tokens.Byte,
 Tokens.Currency,
 Tokens.Date,
 Tokens.Decimal,
 Tokens.Double,
 Tokens.Integer,
 Tokens.Long,
 Tokens.LongLong,
 Tokens.Single,
 Tokens.String
 };
 public static bool IsValueType(string typeName)
 {
 return ValueTypes.Contains(typeName);
 }
 private string GetExtractedMethod()
 {
 const string newLine = "\r\n";
 var access = _view.Accessibility.ToString();
 var keyword = Tokens.Sub;
 var returnType = string.Empty;
 var isFunction = _view.ReturnValue != null && _view.ReturnValue.Name != "(none)";
 if (isFunction)
 {
 keyword = Tokens.Function;
 returnType = Tokens.As + ' ' + _view.ReturnValue.TypeName;
 }
 var parameters = "(" + string.Join(", ", _view.Parameters) + ")";
 var result = access + ' ' + keyword + ' ' + _view.MethodName + parameters + ' ' + returnType + newLine;
 var localConsts = _locals.Where(e => e.DeclarationType == DeclarationType.Constant)
 .Cast<ValuedDeclaration>()
 .Select(e => " " + Tokens.Const + ' ' + e.IdentifierName + ' ' + Tokens.As + ' ' + e.AsTypeName + " = " + e.Value);
 var localVariables = _locals.Where(e => e.DeclarationType == DeclarationType.Variable)
 .Where(e => _view.Parameters.All(param => param.Name != e.IdentifierName))
 .Select(e => e.Context)
 .Cast<VBAParser.VariableSubStmtContext>()
 .Select(e => " " + Tokens.Dim + ' ' + e.ambiguousIdentifier().GetText() + 
 (e.LPAREN() == null 
 ? string.Empty 
 : e.LPAREN().GetText() + (e.subscripts() == null ? string.Empty : e.subscripts().GetText()) + e.RPAREN().GetText()) + ' ' + 
 (e.asTypeClause() == null ? string.Empty : e.asTypeClause().GetText()));
 var locals = string.Join(newLine, localConsts.Union(localVariables)
 .Where(local => !_selectedCode.Contains(local)).ToArray()) + newLine;
 result += locals + _selectedCode + newLine;
 if (isFunction)
 {
 // return value by assigning the method itself:
 var setter = _view.SetReturnValue ? Tokens.Set + ' ' : string.Empty;
 result += " " + setter + _view.MethodName + " = " + _view.ReturnValue.Name + newLine;
 }
 result += Tokens.End + ' ' + keyword + newLine;
 return newLine + result + newLine;
 }
 }
}

• I feel like the constructor is doing too much work. Should I move its logic elsewhere?
• The Show method is... bleh. Should I extract smaller methods from that?
• GetExtractedMethod is... beyond bleh. How can I turn this ugliness into something pretty?
• Anything else?

A tremendous amount of things have changed since version 1.2, when this feature was first introduced - the more important thing being the advent of the Declarations API, which implies that the feature now not only gets an ANTLR parse tree to play with, but also a strongly-typed IEnumerable<Declaration> collection, each with their References resolved (see this post for more details on that specific part).

namespace Rubberduck.UI.Refactorings.ExtractMethod
{
 public class ExtractMethodPresenter
 {
 private readonly IExtractMethodDialog _view;
 private readonly IEnumerable<ExtractedParameter> _input;
 private readonly IEnumerable<ExtractedParameter> _output;
 private readonly List<Declaration> _locals;
 private readonly List<Declaration> _toRemoveFromSource;
 private readonly string _selectedCode;
 private readonly QualifiedSelection _selection;
 private readonly IActiveCodePaneEditor _editor;
 private readonly Declaration _member;
 private readonly HashSet<Declaration> _usedInSelection;
 private readonly HashSet<Declaration> _usedBeforeSelection;
 private readonly HashSet<Declaration> _usedAfterSelection;
 public ExtractMethodPresenter(IActiveCodePaneEditor editor, IExtractMethodDialog view, Declaration member, QualifiedSelection selection, Declarations declarations)
 {
 _editor = editor;
 _view = view;
 _member = member;
 _selection = selection;
 _selectedCode = _editor.GetLines(selection.Selection);
 var inScopeDeclarations = declarations.Items.Where(item => item.ParentScope == member.Scope).ToList();
 var inSelection = inScopeDeclarations.SelectMany(item => item.References)
 .Where(item => selection.Selection.Contains(item.Selection))
 .ToList();
 _usedInSelection = new HashSet<Declaration>(inScopeDeclarations.Where(item =>
 item.References.Any(reference => inSelection.Contains(reference))));
 _usedBeforeSelection = new HashSet<Declaration>(inScopeDeclarations.Where(item => 
 item.References.Any(reference => reference.Selection.StartLine < selection.Selection.StartLine)));
 _usedAfterSelection = new HashSet<Declaration>(inScopeDeclarations.Where(item =>
 item.References.Any(reference => reference.Selection.StartLine > selection.Selection.EndLine)));
 // identifiers used inside selection and before selection (or if it's a parameter) are candidates for parameters:
 var input = inScopeDeclarations.Where(item => 
 _usedInSelection.Contains(item) && (_usedBeforeSelection.Contains(item) || item.DeclarationType == DeclarationType.Parameter)).ToList();
 // identifiers used inside selection and after selection are candidates for return values:
 var output = inScopeDeclarations.Where(item => 
 _usedInSelection.Contains(item) && _usedAfterSelection.Contains(item))
 .ToList();
 // identifiers used only inside and/or after selection are candidates for locals:
 _locals = inScopeDeclarations.Where(item => item.DeclarationType != DeclarationType.Parameter && (
 item.References.All(reference => inSelection.Contains(reference))
 || (_usedAfterSelection.Contains(item) && (!_usedBeforeSelection.Contains(item)))))
 .ToList();
 // locals that are only used in selection are candidates for being moved into the new method:
 _toRemoveFromSource = _locals.Where(item => !_usedAfterSelection.Contains(item)).ToList();
 _output = output.Select(declaration =>
 new ExtractedParameter(declaration.AsTypeName, ExtractedParameter.PassedBy.ByRef, declaration.IdentifierName));
 _input = input.Where(declaration => !output.Contains(declaration))
 .Select(declaration =>
 new ExtractedParameter(declaration.AsTypeName, ExtractedParameter.PassedBy.ByVal, declaration.IdentifierName));
 }
 public void Show()
 {
 PrepareView();
 if (_view.ShowDialog() != DialogResult.OK)
 {
 return;
 }
 ExtractMethod();
 }
 private void PrepareView()
 {
 _view.MethodName = "Method1";
 _view.Inputs = _input.ToList();
 _view.Outputs = _output.ToList();
 _view.Locals = _locals.Select(variable => new ExtractedParameter(variable.AsTypeName, ExtractedParameter.PassedBy.ByVal, variable.IdentifierName)).ToList();
 var returnValues = new[] {new ExtractedParameter(string.Empty, ExtractedParameter.PassedBy.ByVal)}
 .Union(_view.Outputs)
 .Union(_view.Inputs)
 .ToList();
 _view.ReturnValues = returnValues;
 _view.ReturnValue = (_output.Any() && !_output.Skip(1).Any())
 ? _output.Single()
 : returnValues.First();
 _view.RefreshPreview += _view_RefreshPreview;
 _view.OnRefreshPreview();
 }
 private void ExtractMethod()
 {
 _editor.DeleteLines(_selection.Selection);
 _editor.ReplaceLine(_selection.Selection.StartLine, GetMethodCall());
 var insertionLine = _member.Context.GetSelection().EndLine - _selection.Selection.LineCount + 2;
 _editor.InsertLines(insertionLine, GetExtractedMethod());
 // assumes these are declared *before* the selection...
 var offset = 0;
 foreach (var declaration in _toRemoveFromSource.OrderBy(e => e.Selection.StartLine))
 {
 var target = new Selection(
 declaration.Selection.StartLine - offset,
 declaration.Selection.StartColumn,
 declaration.Selection.EndLine - offset,
 declaration.Selection.EndColumn);
 _editor.DeleteLines(target);
 offset += declaration.Selection.LineCount;
 }
 }
 private static readonly IEnumerable<string> ValueTypes = new[]
 {
 Tokens.Boolean,
 Tokens.Byte,
 Tokens.Currency,
 Tokens.Date,
 Tokens.Decimal,
 Tokens.Double,
 Tokens.Integer,
 Tokens.Long,
 Tokens.LongLong,
 Tokens.Single,
 Tokens.String
 };
 private void _view_RefreshPreview(object sender, EventArgs e)
 {
 var hasReturnValue = _view.ReturnValue != null && _view.ReturnValue.Name != ExtractedParameter.None;
 _view.CanSetReturnValue =
 hasReturnValue && !ValueTypes.Contains(_view.ReturnValue.TypeName);
 GeneratePreview();
 }
 private void GeneratePreview()
 {
 _view.Preview = GetExtractedMethod();
 }
 private string GetMethodCall()
 {
 string result;
 var returnValueName = _view.ReturnValue.Name;
 var argsList = string.Join(", ", _view.Parameters.Select(p => p.Name));
 if (returnValueName != ExtractedParameter.None)
 {
 var setter = _view.SetReturnValue ? Tokens.Set + ' ' : string.Empty;
 result = setter + returnValueName + " = " + _view.MethodName + '(' + argsList + ')';
 }
 else
 {
 result = _view.MethodName + ' ' + argsList;
 }
 return " " + result; // todo: smarter indentation
 }
 private string GetExtractedMethod()
 {
 var newLine = Environment.NewLine;
 var access = _view.Accessibility.ToString();
 var keyword = Tokens.Sub;
 var asTypeClause = string.Empty;
 var isFunction = _view.ReturnValue != null && _view.ReturnValue.Name != ExtractedParameter.None;
 if (isFunction)
 {
 keyword = Tokens.Function;
 asTypeClause = Tokens.As + ' ' + _view.ReturnValue.TypeName;
 }
 var parameters = "(" + string.Join(", ", _view.Parameters) + ")";
 var result = access + ' ' + keyword + ' ' + _view.MethodName + parameters + ' ' + asTypeClause + newLine;
 var localConsts = _locals.Where(e => e.DeclarationType == DeclarationType.Constant)
 .Cast<ValuedDeclaration>()
 .Select(e => " " + Tokens.Const + ' ' + e.IdentifierName + ' ' + Tokens.As + ' ' + e.AsTypeName + " = " + e.Value);
 var localVariables = _locals.Where(e => e.DeclarationType == DeclarationType.Variable)
 .Where(e => _view.Parameters.All(param => param.Name != e.IdentifierName))
 .Select(e => e.Context)
 .Cast<VBAParser.VariableSubStmtContext>()
 .Select(e => " " + Tokens.Dim + ' ' + e.ambiguousIdentifier().GetText() + 
 (e.LPAREN() == null 
 ? string.Empty 
 : e.LPAREN().GetText() + (e.subscripts() == null ? string.Empty : e.subscripts().GetText()) + e.RPAREN().GetText()) + ' ' + 
 (e.asTypeClause() == null ? string.Empty : e.asTypeClause().GetText()));
 var locals = string.Join(newLine, localConsts.Union(localVariables)
 .Where(local => !_selectedCode.Contains(local)).ToArray()) + newLine;
 result += locals + _selectedCode + newLine;
 if (isFunction)
 {
 // return value by assigning the method itself:
 var setter = _view.SetReturnValue ? Tokens.Set + ' ' : string.Empty;
 result += " " + setter + _view.MethodName + " = " + _view.ReturnValue.Name + newLine;
 }
 result += Tokens.End + ' ' + keyword + newLine;
 return newLine + result + newLine;
 }
 }
}

• I feel like the constructor is doing too much work. Should I move its logic elsewhere?
• GetExtractedMethod is... bleh. How can I turn this ugliness into something pretty?
• Anything else?