5
\$\begingroup\$

The latest refactoring for Rubberduck is Implement Interface. This refactoring will check whether the active class fully implements the selected interface by comparing the definitions. It knows the class based on the Implements statement, which looks like Implements IXyz.

This extension method finds the interface based on the statement, which is a reference to the interface class. The implementsStmt contains the 'Implements' keyword, and the reference variable contains the name of the interface. I need to combine the two selections for user selections such as "Impl[ements IX]yz", with the braces showing the beginning and end of the selection. I return null if a statement is not found that contains the selection

[SuppressMessage("ReSharper", "LoopCanBeConvertedToQuery")]
public static Declaration FindInterface(this IEnumerable<Declaration> declarations, QualifiedSelection selection)
{
    foreach (var declaration in declarations.FindInterfaces())
    {
        foreach (var reference in declaration.References)
        {
            var implementsStmt = reference.Context.Parent as VBAParser.ImplementsStmtContext;

            if (implementsStmt == null) { continue; }

            var completeSelection = new Selection(implementsStmt.GetSelection().StartLine,
                implementsStmt.GetSelection().StartColumn, reference.Selection.EndLine,
                reference.Selection.EndColumn);

            if (reference.QualifiedModuleName.ComponentName == selection.QualifiedName.ComponentName &&
                reference.QualifiedModuleName.Project == selection.QualifiedName.Project &&
                completeSelection.Contains(selection.Selection))
            {
                return declaration;
            }
        }
    }

    return null;
}

This is the refactoring itself. My main concern here is how I alternate between using the Tokens class and manually typing out the string. I prefer using the Tokens class to prevent typos, &c., but I use a string literal when I would need to concatenate several tokens to save space. Should I be using just one or the other here? Feel free to comment on any other concern as well.

public class ImplementInterfaceRefactoring : IRefactoring
{
    private readonly List<Declaration> _declarations;
    private readonly IActiveCodePaneEditor _editor;
    private Declaration _targetInterface;
    private Declaration _targetClass;
    private readonly IMessageBox _messageBox;

    const string MemberBody = "    Err.Raise 5";

    public ImplementInterfaceRefactoring(RubberduckParserState state, IActiveCodePaneEditor editor, IMessageBox messageBox)
    {
        _declarations = state.AllDeclarations.ToList();
        _editor = editor;
        _messageBox = messageBox;
    }

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

        if (!selection.HasValue)
        {
            _messageBox.Show(RubberduckUI.ImplementInterface_InvalidSelectionMessage, RubberduckUI.ImplementInterface_Caption,
                System.Windows.Forms.MessageBoxButtons.OK, System.Windows.Forms.MessageBoxIcon.Exclamation);
            return;
        }

        Refactor(selection.Value);
    }

    public void Refactor(QualifiedSelection selection)
    {
        _targetInterface = _declarations.FindInterface(selection);

        _targetClass = _declarations.SingleOrDefault(d =>
                    !d.IsBuiltIn && d.DeclarationType == DeclarationType.Class &&
                    d.QualifiedSelection.QualifiedName.ComponentName == selection.QualifiedName.ComponentName &&
                    d.Project == selection.QualifiedName.Project);

        if (_targetClass == null || _targetInterface == null)
        {
            _messageBox.Show(RubberduckUI.ImplementInterface_InvalidSelectionMessage, RubberduckUI.ImplementInterface_Caption,
                System.Windows.Forms.MessageBoxButtons.OK, System.Windows.Forms.MessageBoxIcon.Exclamation);
            return;
        }

        ImplementMissingMembers();
    }

    public void Refactor(Declaration target)
    {
        throw new NotImplementedException();
    }

    private void ImplementMissingMembers()
    {
        var interfaceMembers = GetInterfaceMembers();
        var implementedMembers = GetImplementedMembers();

        var nonImplementedMembers =
            interfaceMembers.Where(
                d =>
                    !implementedMembers.Select(s => s.IdentifierName)
                        .Contains(_targetInterface.ComponentName + "_" + d.IdentifierName)).ToList();

        AddItems(nonImplementedMembers);
    }

    private void AddItems(List<Declaration> members)
    {
        var module = _targetClass.QualifiedSelection.QualifiedName.Component.CodeModule;

        members.Reverse();

        foreach (var member in members)
        {
            module.InsertLines(module.CountOfDeclarationLines + 1, GetInterfaceMember(member));
        }
    }

    private string GetInterfaceMember(Declaration member)
    {
        switch (GetMemberType(member))
        {
            case "Sub":
                return SubStmt(member);

            case "Function":
                return FunctionStmt(member);

            case "Property Get":
                return PropertyGetStmt(member);

            case "Property Let":
                return PropertyLetStmt(member);

            case "Property Set":
                return PropertySetStmt(member);
        }

        return string.Empty;
    }

    private string SubStmt(Declaration member)
    {
        var memberParams = GetParameters(member);

        var memberSignature = "Public Sub " + _targetInterface.IdentifierName + "_" + member.IdentifierName + "(" +
                              string.Join(", ", memberParams) + ")";

        var memberCloseStatement = "End Sub" + Environment.NewLine;

        return string.Join(Environment.NewLine, memberSignature, MemberBody, memberCloseStatement);
    }

    private string FunctionStmt(Declaration member)
    {
        var memberParams = GetParameters(member);

        var memberSignature = "Public Function " + _targetInterface.IdentifierName + "_" + member.IdentifierName + "(" +
                              string.Join(", ", memberParams) + ")" + " As " + member.AsTypeName;

        var memberCloseStatement = "End Function" + Environment.NewLine;

        return string.Join(Environment.NewLine, memberSignature, MemberBody, memberCloseStatement);
    }

    private string PropertyGetStmt(Declaration member)
    {
        var memberParams = GetParameters(member);

        var memberSignature = "Public Property Get " + _targetInterface.IdentifierName + "_" + member.IdentifierName + "(" +
                              string.Join(", ", memberParams) + ")" + " As " + member.AsTypeName;

        var memberCloseStatement = "End Property" + Environment.NewLine;

        return string.Join(Environment.NewLine, memberSignature, MemberBody, memberCloseStatement);
    }

    private string PropertyLetStmt(Declaration member)
    {
        var memberParams = GetParameters(member);

        var memberSignature = "Public Property Let " + _targetInterface.IdentifierName + "_" + member.IdentifierName +
                              "(" + string.Join(", ", memberParams) + ")";

        var memberCloseStatement = "End Property" + Environment.NewLine;

        return string.Join(Environment.NewLine, memberSignature, MemberBody, memberCloseStatement);
    }

    private string PropertySetStmt(Declaration member)
    {
        var memberParams = GetParameters(member);

        var memberSignature = "Public Property Set " + _targetInterface.IdentifierName + "_" + member.IdentifierName +
                              "(" + string.Join(", ", memberParams) + ")";

        var memberCloseStatement = "End Property" + Environment.NewLine;

        return string.Join(Environment.NewLine, memberSignature, MemberBody, memberCloseStatement);
    }

    private List<Parameter> GetParameters(Declaration member)
    {
        var parameters = _declarations.Where(item => item.DeclarationType == DeclarationType.Parameter &&
                          item.ParentScope == member.Scope)
                       .OrderBy(o => o.Selection.StartLine)
                       .ThenBy(t => t.Selection.StartColumn)
                       .Select(p => new Parameter
                       {
                           ParamAccessibility = ((VBAParser.ArgContext)p.Context).BYREF() == null ? Tokens.ByVal : Tokens.ByRef,
                           ParamName = p.IdentifierName,
                           ParamType = p.AsTypeName
                       })
                       .ToList();

        if (member.DeclarationType == DeclarationType.PropertyGet)
        {
            parameters.Remove(parameters.Last());
        }

        return parameters;
    }

    private IEnumerable<Declaration> GetInterfaceMembers()
    {
        return _declarations.FindInterfaceMembers()
                            .Where(d => d.ComponentName == _targetInterface.IdentifierName)
                            .OrderBy(d => d.Selection.StartLine)
                            .ThenBy(d => d.Selection.StartColumn);
    }

    private IEnumerable<Declaration> GetImplementedMembers()
    {
        return _declarations.FindInterfaceImplementationMembers()
                            .Where(item => item.Project.Equals(_targetInterface.Project)
                                    && item.ComponentName == _targetClass.IdentifierName
                                    && item.IdentifierName.StartsWith(_targetInterface.ComponentName + "_")
                                    && !item.Equals(_targetClass))
                            .OrderBy(d => d.Selection.StartLine)
                            .ThenBy(d => d.Selection.StartColumn);
    }

    private string GetMemberType(Declaration member)
    {
        var context = member.Context;

        var subStmtContext = context as VBAParser.SubStmtContext;
        if (subStmtContext != null)
        {
            return Tokens.Sub;
        }

        var functionStmtContext = context as VBAParser.FunctionStmtContext;
        if (functionStmtContext != null)
        {
            return Tokens.Function;
        }

        var propertyGetStmtContext = context as VBAParser.PropertyGetStmtContext;
        if (propertyGetStmtContext != null)
        {
            return Tokens.Property + " " + Tokens.Get;
        }

        var propertyLetStmtContext = context as VBAParser.PropertyLetStmtContext;
        if (propertyLetStmtContext != null)
        {
            return Tokens.Property + " " + Tokens.Let;
        }

        var propertySetStmtContext = context as VBAParser.PropertySetStmtContext;
        if (propertySetStmtContext != null)
        {
            return Tokens.Property + " " + Tokens.Set;
        }

        return string.Empty;
    }
}
\$\endgroup\$
2
\$\begingroup\$

FindInterface()

There is no need to create the Selection completeSelection if the ComponentName or the Project don't match. So better create the completeSelection inside the if and add another if condition for the check if completeSelection.Contains(selection.Selection) like so

[SuppressMessage("ReSharper", "LoopCanBeConvertedToQuery")]
public static Declaration FindInterface(this IEnumerable<Declaration> declarations, QualifiedSelection selection)
{
    foreach (var declaration in declarations.FindInterfaces())
    {
        foreach (var reference in declaration.References)
        {
            var implementsStmt = reference.Context.Parent as VBAParser.ImplementsStmtContext;

            if (implementsStmt == null) { continue; }

            if (reference.QualifiedModuleName.ComponentName == selection.QualifiedName.ComponentName &&
                reference.QualifiedModuleName.Project == selection.QualifiedName.Project)
            {

                var completeSelection = new Selection(implementsStmt.GetSelection().StartLine,  
                    implementsStmt.GetSelection().StartColumn, reference.Selection.EndLine,  
                    reference.Selection.EndColumn);

                if (completeSelection.Contains(selection.Selection))
                {
                    return declaration;
                }
            }
        }
    }

    return null;
}

ImplementInterfaceRefactoring

In the Refactor() method it isn't obvious that the returned value from the call to GetSelection() is a nullable QualifiedSelection so the usage of the var type isn't good.

IMO the class shouldn't interact with the UI by showing some messagebox. Why don't you return a state from the Refactor() methods indicating failure/success ?
In that way it could be tested much better.

GetInterfaceMember()

Instead of the switch statement I would like to suggest using a Dictionary<string, Func<string, Declaration>>.

That would reduce the method to something like this

private void InitializeMemberDictionary()
{
    dict = new Dictionary<string, Func<string, Declaration>>();
    dict.Add("Sub", SubStmt(member));
    dict.Add(.....
    ....
    ....
    ....
}
private Dictionary<string, Func<string, Declaration>> dict;
private string GetInterfaceMember(Declaration member)
{
    string memberType = GetMemberType(member);
    Func<string, Declaration> func;
    if (!dict.TryGetValue(memberType, out func))  
    {
        return string.Empty;
    } 

    return func(member);
}
\$\endgroup\$
  • \$\begingroup\$ We use a mock messagebox for testing. We have no problem not displaying the box during tests. \$\endgroup\$ – Hosch250 Dec 29 '15 at 15:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.