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

Implement Interface Refactoring

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

Problem

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

Solution

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

That would reduce the method to something like this

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

    return func(member);
}

Code Snippets

[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;
}
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);
}

Context

StackExchange Code Review Q#115042, answer score: 2

Revisions (0)

No revisions yet.