12
\$\begingroup\$

Our Rubberduck open-source VBE add-in project is coming along nicely. One of the main features we're going to be implementing in the next release, is code inspections.

early-stage screenshot of a window titled "Code Inspections" showing a "refresh" button on a toolbar, and a list with an item that says "Replace Rem reserved keyword with single quote.", referring to a line of code that says "Rem this is an obsolete-syntax comment"

I started by defining an abstraction:

namespace Rubberduck.Inspections
{
    [ComVisible(false)]
    public enum CodeInspectionSeverity
    {
        DoNotShow,
        Hint,
        Suggestion,
        Warning,
        Error
    }

    [ComVisible(false)]
    public enum CodeInspectionType
    {
        MaintainabilityAndReadabilityIssues,
        CodeQualityIssues
    }

    /// <summary>
    /// An interface that abstracts a code inspection.
    /// </summary>
    [ComVisible(false)]
    public interface IInspection
    {
        /// <summary>
        /// Gets a short description for the code inspection.
        /// </summary>
        string Name { get; }

        /// <summary>
        /// Gets a short message that describes how a code issue can be fixed.
        /// </summary>
        string QuickFixMessage { get; }

        /// <summary>
        /// Gets a value indicating the type of the code inspection.
        /// </summary>
        CodeInspectionType InspectionType { get; }

        /// <summary>
        /// Gets a value indicating the severity level of the code inspection.
        /// </summary>
        CodeInspectionSeverity Severity { get; }

        /// <summary>
        /// Gets/sets a valud indicating whether the inspection is enabled or not.
        /// </summary>
        bool IsEnabled { get; set; }

        /// <summary>
        /// Runs code inspection on specified tree node (and child nodes).
        /// </summary>
        /// <param name="node">The <see cref="SyntaxTreeNode"/> to analyze.</param>
        /// <returns>Returns inspection results, if any.</returns>
        IEnumerable<CodeInspectionResultBase> Inspect(SyntaxTreeNode node);
    }
}

Out of necessity came a CodeInspection base class to implement it:

namespace Rubberduck.Inspections
{
    [ComVisible(false)]
    public abstract class CodeInspection : IInspection
    {
        protected CodeInspection(string name, string message, CodeInspectionType type, CodeInspectionSeverity severity)
        {
            _name = name;
            _message = message;
            _inspectionType = type;
            Severity = severity;
        }

        private readonly string _name;
        public string Name { get { return _name; } }

        private readonly string _message;
        public string QuickFixMessage { get { return _message; } }

        private readonly CodeInspectionType _inspectionType;
        public CodeInspectionType InspectionType { get { return _inspectionType; } }

        public CodeInspectionSeverity Severity { get; set; }
        public bool IsEnabled { get; set; }

        /// <summary>
        /// Inspects specified tree node, searching for code issues.
        /// </summary>
        /// <param name="node"></param>
        /// <returns></returns>
        public abstract IEnumerable<CodeInspectionResultBase> Inspect(SyntaxTreeNode node);
    }
}

The first implementation I wrote was ObsoleteCommentSyntaxInspection, which looks for usages of the obsolete Rem keyword in comments:

namespace Rubberduck.Inspections
{
    [ComVisible(false)]
    public class ObsoleteCommentSyntaxInspection : CodeInspection
    {
        /// <summary>
        /// Parameterless constructor required for discovery of implemented code inspections.
        /// </summary>
        public ObsoleteCommentSyntaxInspection()
            : base("Use of obsolete Rem comment syntax", 
                   "Replace Rem reserved keyword with single quote.", 
                   CodeInspectionType.MaintainabilityAndReadabilityIssues, 
                   CodeInspectionSeverity.Suggestion)
        {
        }

        public override IEnumerable<CodeInspectionResultBase> Inspect(SyntaxTreeNode node)
        {
            var comments = node.FindAllComments();
            var remComments = comments.Where(instruction => instruction.Value.Trim().StartsWith(ReservedKeywords.Rem));
            return remComments.Select(instruction => new ObsoleteCommentSyntaxInspectionResult(Name, instruction, Severity, QuickFixMessage));
        }
    }
}

When an IInspection implementation finds an Instruction noteworthy, it creates an instance of a class derived from CodeInspectionResultBase:

namespace Rubberduck.Inspections
{
    [ComVisible(false)]
    public abstract class CodeInspectionResultBase
    {
        public CodeInspectionResultBase(string inspection, Instruction instruction, CodeInspectionSeverity type, string message)
        {
            _name = inspection;
            _instruction = instruction;
            _type = type;
            _message = message;
        }

        private readonly string _name;
        /// <summary>
        /// Gets a string containing the name of the code inspection.
        /// </summary>
        public string Name { get { return _name; } }

        private readonly Instruction _instruction;
        /// <summary>
        /// Gets the <see cref="Instruction"/> containing a code issue.
        /// </summary>
        public Instruction Instruction { get { return _instruction; } }

        private readonly CodeInspectionSeverity _type;
        /// <summary>
        /// Gets the severity of the code issue.
        /// </summary>
        public CodeInspectionSeverity Severity { get { return _type; } }

        private readonly string _message;
        /// <summary>
        /// Gets a short message that describes how the code issue can be fixed.
        /// </summary>
        public string Message { get { return _message; } }

        /// <summary>
        /// Addresses the issue by making changes to the code.
        /// </summary>
        /// <param name="vbe"></param>
        public abstract void QuickFix(VBE vbe);
    }
}

Having this base class makes implementing the QuickFix method, the only thing left to implement:

namespace Rubberduck.Inspections
{
    public class ObsoleteCommentSyntaxInspectionResult : CodeInspectionResultBase
    {
        public ObsoleteCommentSyntaxInspectionResult(string inspection, Instruction instruction, CodeInspectionSeverity type, string message) 
            : base(inspection, instruction, type, message)
        {
        }

        public override void QuickFix(VBE vbe)
        {
            var location = vbe.FindInstruction(Instruction);
            int index;
            if (!Instruction.Line.Content.HasComment(out index)) return;

            var line = Instruction.Line.Content.Substring(0, index) + "'" + Instruction.Comment.Substring(ReservedKeywords.Rem.Length);
            location.CodeModule.ReplaceLine(location.Selection.StartLine, line);
        }
    }
}

We're planning to implement a good number of such code inspections - is this a good, maintainable, extensible way to go about it?

The available inspections are loaded at startup, like this:

_inspections = Assembly.GetExecutingAssembly()
                       .GetTypes()
                       .Where(type => type.BaseType ==  typeof(CodeInspection))
                       .Select(type =>
                       {
                           var constructor = type.GetConstructor(Type.EmptyTypes);
                           return constructor != null ? constructor.Invoke(Type.EmptyTypes) : null;
                       })
                      .Where(inspection => inspection != null)
                       .Cast<IInspection>()
                       .ToList();

This way as we implement them, they're automagically effective.

I'm hoping that this design will help us fully implement this feature faster, by writing only the code that needs to be written. Hence I'm interested in extensibility, maintainability and readability in general - but also in how I've put these abstractions into use, in the CodeInspectionsDockablePresenter class:

[ComVisible(false)]
public class CodeInspectionsDockablePresenter : DockablePresenterBase
{
    private readonly Parser _parser;
    private CodeInspectionsWindow Control { get { return UserControl as CodeInspectionsWindow; } }

    private readonly IList<IInspection> _inspections;

    public CodeInspectionsDockablePresenter(Parser parser, IEnumerable<IInspection> inspections, VBE vbe, AddIn addin) 
        : base(vbe, addin, new CodeInspectionsWindow())
    {
        _parser = parser;
        _inspections = inspections.ToList();

        Control.RefreshCodeInspections += OnRefreshCodeInspections;
        Control.NavigateCodeIssue += OnNavigateCodeIssue;
    }

    private void OnNavigateCodeIssue(object sender, NavigateCodeIssueEventArgs e)
    {
        var location = VBE.FindInstruction(e.Instruction);
        location.CodeModule.CodePane.SetSelection(location.Selection);
    }

    private void OnRefreshCodeInspections(object sender, EventArgs e)
    {
        var code = _parser.Parse(VBE.ActiveVBProject);
        var results = new List<CodeInspectionResultBase>();
        foreach (var inspection in _inspections.Where(inspection => inspection.IsEnabled))
        {
            var result = inspection.Inspect(code).ToArray();
            if (result.Length != 0)
            {
                results.AddRange(result);
            }
        }

        DrawResultTree(results);
    }

    private void DrawResultTree(IEnumerable<CodeInspectionResultBase> results)
    {
        var tree = Control.CodeInspectionResultsTree;
        tree.Nodes.Clear();

        foreach (var result in results.OrderBy(r => r.Severity))
        {
            var node = new TreeNode(result.Name);
            node.ToolTipText = result.Instruction.Content;
            node.Tag = result.Instruction;
            tree.Nodes.Add(node);
        }
    }
}

Nevermind DrawResultTree, it's there just because I wanted to see something in the dockable window; it's mostly OnRefreshCodeInspections I'm interested in - are there any obvious (or not) optimizations that could be operated in the looping?

\$\endgroup\$
12
\$\begingroup\$

Having worked on a similar system, I came up with a very similar design. So either we're both doing something right, or we're both doing it wrong :)

Here are some things I would change, but obviously different requirements call for different decisions and may not all be applicable here.

Remove CodeInspection class

I don't think you gain much from having a base class here. Now your inspections look more like this

public class ObsoleteCommentSyntaxInspection : IInspection
{
    public string Name
    {
        get { return Properties.Resources.ObsoleteCommentSyntaxInspectionName; }
    }

    public CodeInspectionType InspectionType
    {
        get { return CodeInspectionType.MaintainabilityAndReadabilityIssues; }
    }

    public CodeInspectionSeverity Severity
    {
        get { return CodeInspectionSeverity.Suggestion; }
    }

    ...

Remove IsEnabled property

I think enabling/disabling inspections should be handled at a different level. Especially if you want something more complex like a project-specific settings file overriding a global one, something like an InspectionSettings class would be good here.

Inspections might have more than one fix (or none)

Consider returning an IEnumerable<string> for quick fixes.

Rename Inspect

The return type is IEnumerable<CodeInspectionResultBase>, so I would recommend something like GetInspectionResults.

\$\endgroup\$
  • 3
    \$\begingroup\$ Wish I could give more than +1 for the IsEnabled bit alone. Whether some other part of the code decides to call an object's methods is the concern of the calling code, not the object. Classes should tend to their own concerns, not be designed with the entire overall structure in mind. \$\endgroup\$ – itsbruce Nov 30 '14 at 8:41
  • \$\begingroup\$ Good catch wih IsEnabled; I originally didn't have a NotShown severity level, which should deprecate that IsEnabled flag. \$\endgroup\$ – Mathieu Guindon Nov 30 '14 at 14:05

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.