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

Exploring the code of the Code Explorer

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codereviewmvpcsharpstackoverflowwinformsc#rubberduck
codeexploringexplorerthe

Problem

The Code Explorer is a dockable toolwindow that displays a TreeView that shows all opened VBA projects and their respective modules, but unlike the "native" Project Explorer, Rubberduck's explorer also drills down to individual module members, and allows quickly navigating them:

While it looks pretty, the code behind it is among the most painful code in the project - clearly, it needs to be refactored and optimized. I'm particularly interested in ways to improve the async void RefreshExplorerTreeView() method and the whole node-generation code.

```
namespace Rubberduck.UI.CodeExplorer
{
public class CodeExplorerDockablePresenter : DockablePresenterBase
{
private readonly IRubberduckParser _parser;
private CodeExplorerWindow Control { get { return UserControl as CodeExplorerWindow; } }

public CodeExplorerDockablePresenter(IRubberduckParser parser, VBE vbe, AddIn addIn, CodeExplorerWindow view)
: base(vbe, addIn, view)
{
_parser = parser;
RegisterControlEvents();
}

public override void Show()
{
base.Show();
Task.Run(() => RefreshExplorerTreeView());
}

private void RegisterControlEvents()
{
if (Control == null)
{
return;
}

Control.RefreshTreeView += RefreshExplorerTreeView;
Control.NavigateTreeNode += NavigateExplorerTreeNode;
Control.AddComponent += AddComponent;
Control.AddTestModule += AddTestModule;
Control.ToggleFolders += ToggleFolders;
Control.ShowDesigner += ShowDesigner;
Control.DisplayStyleChanged += DisplayStyleChanged;
Control.RunAllTests += ContextMenuRunAllTests;
Control.RunInspections += ContextMenuRunInspections;
Control.SelectionChanged += SelectionChanged;
Control.Rename += RenameSelection;
Control.FindAl

Solution

(Not a full review, just focusing on one method.)

I think private string GetImageKeyForDeclaration(Declaration declaration) contains a lot of repeated code that can be significantly reduced.

First of all I'm not a fan of the "use a switch to set a value that you then return"-approach: just do the return directly, especially if there are a lot of cases.

I'd group all of these:

case DeclarationType.Module:
    break;
case DeclarationType.Class:
    break;
case DeclarationType.Parameter:
    break;


So that would become:

case DeclarationType.Module:
case DeclarationType.Class:
case DeclarationType.Parameter:
    return string.Empty;


Each of these I'd change to a method call:

if (declaration.Accessibility == Accessibility.Private)
{
    result = "PrivateMethod";
    break;
}
if (declaration.Accessibility == Accessibility.Friend)
{
    result = "FriendMethod";
    break;
}
result = "PublicMethod";


So that would become something along these lines:

case DeclarationType.Function:
    return GetImageKeyForDeclaration(declaration, "Method");

case DeclarationType.PropertyGet:
case DeclarationType.PropertyLet:
case DeclarationType.PropertySet:
    return GetImageKeyForDeclaration(declaration, "Property");

private string GetImageKeyForDeclaration(Declaration declaration, string suffix)
{
    if (declaration.Accessibility == Accessibility.Private)
    {
        return "Private" + suffix;
    }
    if (declaration.Accessibility == Accessibility.Friend)
    {
        return "Friend" + suffix;
    }
    return "Public" + suffix;
}


Which means this method is now half its original length:

private string GetImageKeyForDeclaration(Declaration declaration)
{
    switch (declaration.DeclarationType)
    {
        case DeclarationType.Module:
        case DeclarationType.Class:
        case DeclarationType.Parameter:
            return string.Empty;

        case DeclarationType.Procedure:
        case DeclarationType.Function:
            return GetImageKeyForDeclaration(declaration, "Method");

        case DeclarationType.PropertyGet:
        case DeclarationType.PropertyLet:
        case DeclarationType.PropertySet:
            return GetImageKeyForDeclaration(declaration, "Property");

        case DeclarationType.Variable:
            return GetImageKeyForDeclaration(declaration, "Field");

        case DeclarationType.Constant:
            return GetImageKeyForDeclaration(declaration, "Const");

        case DeclarationType.Enumeration:
            return GetImageKeyForDeclaration(declaration, "Enum");

        case DeclarationType.Event:
            return GetImageKeyForDeclaration(declaration, "Event");

        case DeclarationType.UserDefinedType:
            return GetImageKeyForDeclaration(declaration, "Type");

        case DeclarationType.EnumerationMember:
            return "EnumItem";

        case DeclarationType.UserDefinedTypeMember:
            return "PublicField";

        case DeclarationType.LibraryProcedure:
        case DeclarationType.LibraryFunction:
            return "Identifier";

        default:
            throw new ArgumentOutOfRangeException();
    }
}


I'm still inclined to move this to a helper class though, considering that CodeExplorerDockablePresenter is 500+ lines long.

Code Snippets

case DeclarationType.Module:
    break;
case DeclarationType.Class:
    break;
case DeclarationType.Parameter:
    break;
case DeclarationType.Module:
case DeclarationType.Class:
case DeclarationType.Parameter:
    return string.Empty;
if (declaration.Accessibility == Accessibility.Private)
{
    result = "PrivateMethod";
    break;
}
if (declaration.Accessibility == Accessibility.Friend)
{
    result = "FriendMethod";
    break;
}
result = "PublicMethod";
case DeclarationType.Function:
    return GetImageKeyForDeclaration(declaration, "Method");

case DeclarationType.PropertyGet:
case DeclarationType.PropertyLet:
case DeclarationType.PropertySet:
    return GetImageKeyForDeclaration(declaration, "Property");


private string GetImageKeyForDeclaration(Declaration declaration, string suffix)
{
    if (declaration.Accessibility == Accessibility.Private)
    {
        return "Private" + suffix;
    }
    if (declaration.Accessibility == Accessibility.Friend)
    {
        return "Friend" + suffix;
    }
    return "Public" + suffix;
}
private string GetImageKeyForDeclaration(Declaration declaration)
{
    switch (declaration.DeclarationType)
    {
        case DeclarationType.Module:
        case DeclarationType.Class:
        case DeclarationType.Parameter:
            return string.Empty;

        case DeclarationType.Procedure:
        case DeclarationType.Function:
            return GetImageKeyForDeclaration(declaration, "Method");

        case DeclarationType.PropertyGet:
        case DeclarationType.PropertyLet:
        case DeclarationType.PropertySet:
            return GetImageKeyForDeclaration(declaration, "Property");

        case DeclarationType.Variable:
            return GetImageKeyForDeclaration(declaration, "Field");

        case DeclarationType.Constant:
            return GetImageKeyForDeclaration(declaration, "Const");

        case DeclarationType.Enumeration:
            return GetImageKeyForDeclaration(declaration, "Enum");

        case DeclarationType.Event:
            return GetImageKeyForDeclaration(declaration, "Event");

        case DeclarationType.UserDefinedType:
            return GetImageKeyForDeclaration(declaration, "Type");

        case DeclarationType.EnumerationMember:
            return "EnumItem";

        case DeclarationType.UserDefinedTypeMember:
            return "PublicField";

        case DeclarationType.LibraryProcedure:
        case DeclarationType.LibraryFunction:
            return "Identifier";

        default:
            throw new ArgumentOutOfRangeException();
    }
}

Context

StackExchange Code Review Q#90029, answer score: 8

Revisions (0)

No revisions yet.