6
\$\begingroup\$

The Rubberduck code inspections have just seen yet another structural change, hopefully for the better.

The IInspectionModel interface was originally named IInspection; it only exposes the bare-bones inspection properties, those needed by the CodeInspectionSetting class:

public interface IInspectionModel
{
    /// <summary>
    /// Gets the inspection type name.
    /// </summary>
    string Name { get; }

    /// <summary>
    /// Gets the name of the inspection, without the "Inspection" suffix.
    /// </summary>
    string AnnotationName { get; }

    /// <summary>
    /// Gets a short description for the code inspection.
    /// </summary>
    string Description { 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; set; }
}

The CodeInspectionSetting type is XML-serialized into the Rubberduck settings file; this allows us to let the user determine an inspection's Severity level:

[XmlType(AnonymousType = true)]
public class CodeInspectionSetting : IInspectionModel
{
    [XmlAttribute]
    public string Name { get; set; }

    [XmlIgnore]
    public string Description { get; set; } // not serialized because culture-dependent

    [XmlIgnore]
    public string AnnotationName { get; set; }

    [XmlAttribute]
    public CodeInspectionSeverity Severity { get; set; }

    [XmlIgnore]
    public string SeverityLabel
    {
        get { return RubberduckUI.ResourceManager.GetString("CodeInspectionSeverity_" + Severity, RubberduckUI.Culture); }
        set
        {
            foreach (var severity in Enum.GetValues(typeof (CodeInspectionSeverity)))
            {
                if (value == RubberduckUI.ResourceManager.GetString("CodeInspectionSeverity_" + severity, RubberduckUI.Culture))
                {
                    Severity = (CodeInspectionSeverity)severity;
                    return;
                }
            }
        }
    }

    [XmlAttribute]
    public CodeInspectionType InspectionType { get; set; }

    public CodeInspectionSetting()
    {
        //default constructor required for serialization
    }

    public CodeInspectionSetting(string name, string description, CodeInspectionType type, CodeInspectionSeverity severity)
    {
        Name = name;
        Description = description;
        InspectionType = type;
        Severity = severity;
    }

    public CodeInspectionSetting(IInspectionModel inspection)
        : this(inspection.Name, inspection.Description, inspection.InspectionType, inspection.Severity)
    { }
}

The SeverityLabel is used by the settings UI. Does it even belong there?

The IInspection interface exposes additional members, including a method responsible for returning actual inspection results (given a parser state, but that's now an implementation detail):

/// <summary>
/// An interface that abstracts a runnable code inspection.
/// </summary>
public interface IInspection : IInspectionModel, IComparable<IInspection>, IComparable
{
    /// <summary>
    /// Runs code inspection on specified parse trees.
    /// </summary>
    /// <returns>Returns inspection results, if any.</returns>
    IEnumerable<CodeInspectionResultBase> GetInspectionResults();

    /// <summary>
    /// Gets a string that contains additional/meta information about an inspection.
    /// </summary>
    string Meta { get; }
}

Up until two days ago, every code inspection implemented IInspection directly. The recent changes introduce an abstract class, to help remove the redundant code that's common to all implementations:

public abstract class InspectionBase : IInspection
{
    protected readonly RubberduckParserState State;
    protected InspectionBase(RubberduckParserState state)
    {
        State = state;
    }

    public abstract string Description { get; }
    public abstract CodeInspectionType InspectionType { get; }
    public abstract IEnumerable<CodeInspectionResultBase> GetInspectionResults();

    public virtual string Name { get { return GetType().Name; } }
    public virtual CodeInspectionSeverity Severity { get; set; }
    public virtual string Meta { get { return InspectionsUI.ResourceManager.GetString(Name + "Meta"); } }
    // ReSharper disable once UnusedMember.Global: it's referenced in xaml
    public virtual string InspectionTypeName { get { return InspectionsUI.ResourceManager.GetString(InspectionType.ToString()); } }
    public virtual string AnnotationName { get { return Name.Replace("Inspection", string.Empty); } }

    protected virtual IEnumerable<Declaration> Declarations
    {
        get { return State.AllDeclarations.Where(declaration => !declaration.IsInspectionDisabled(AnnotationName)); }
    }

    protected virtual IEnumerable<Declaration> UserDeclarations
    {
        get { return State.AllUserDeclarations.Where(declaration => !declaration.IsInspectionDisabled(AnnotationName)); }
    }

    public int CompareTo(IInspection other)
    {
        return string.Compare(InspectionType + Name, other.InspectionType + other.Name, StringComparison.Ordinal);
    }

    public int CompareTo(object obj)
    {
        return CompareTo(obj as IInspection);
    }
}

The main reason for introducing this base class, was to enable the @Ignore {InspectionName} annotations that the IgnoreOnceQuickFix is inserting, without having to specify in every single inspection that it needs to check for IsInspectionDisabled(AnnotationName). Another reason was to avoid the redundant filtering on !declaration.IsBuiltIn, since most inspections operate on user declarations and their usages.

This is the quick fix class in question:

public class IgnoreOnceQuickFix : CodeInspectionQuickFix
{
    private readonly string _annotationText;
    private readonly string _inspectionName;

    public IgnoreOnceQuickFix(ParserRuleContext context, QualifiedSelection selection, string inspectionName) 
        : base(context, selection, InspectionsUI.IgnoreOnce)
    {
        _inspectionName = inspectionName;
        _annotationText = "'" + Parsing.Grammar.Annotations.AnnotationMarker +
                          Parsing.Grammar.Annotations.IgnoreInspection + ' ' + inspectionName;
    }

    public override bool CanFixInModule { get { return false; } } // not quite "once" if applied to entire module
    public override bool CanFixInProject { get { return false; } } // use "disable this inspection" instead of ignoring across the project

    public override void Fix()
    {
        var codeModule = Selection.QualifiedName.Component.CodeModule;
        var insertLine = Selection.Selection.StartLine;

        var codeLine = insertLine == 1 ? string.Empty : codeModule.get_Lines(insertLine - 1, 1);
        var annotationText = _annotationText;
        var ignoreAnnotation = "'" + Parsing.Grammar.Annotations.AnnotationMarker + Parsing.Grammar.Annotations.IgnoreInspection;

        int commentStart;
        if (codeLine.HasComment(out commentStart) && codeLine.Substring(commentStart).StartsWith(ignoreAnnotation))
        {
            annotationText = codeLine + ' ' + _inspectionName;
            codeModule.ReplaceLine(insertLine - 1, annotationText);
        }
        else
        {
            codeModule.InsertLines(insertLine, annotationText);
        }
    }
}

For context, here's an implementation - the ImplicitPublicMemberInspection, which finds public members without an explicit access modifier:

public sealed class ImplicitPublicMemberInspection : InspectionBase
{
    public ImplicitPublicMemberInspection(RubberduckParserState state)
        : base(state)
    {
        Severity = CodeInspectionSeverity.Warning;
    }

    public override string Description { get { return RubberduckUI.ImplicitPublicMember_; } }
    public override CodeInspectionType InspectionType { get { return CodeInspectionType.MaintainabilityAndReadabilityIssues; } }

    private static readonly DeclarationType[] ProcedureTypes = 
    {
        DeclarationType.Function,
        DeclarationType.Procedure,
        DeclarationType.PropertyGet,
        DeclarationType.PropertyLet,
        DeclarationType.PropertySet
    };

    public override IEnumerable<CodeInspectionResultBase> GetInspectionResults()
    {
        var issues = from item in UserDeclarations
                     where ProcedureTypes.Contains(item.DeclarationType)
                        && item.Accessibility == Accessibility.Implicit
                     let context = new QualifiedContext<ParserRuleContext>(item.QualifiedName, item.Context)
                     select new ImplicitPublicMemberInspectionResult(this, string.Format(Description, ((dynamic)context.Context).ambiguousIdentifier().GetText()), context);
        return issues;
    }
}

All inspection classes are sealed, and pass the RubberduckParserState dependency down the base type's constructor.

For completeness' sake, here's the accompanying ImplicitPublicMemberInspectionResult class:

public class ImplicitPublicMemberInspectionResult : CodeInspectionResultBase
{
    private readonly IEnumerable<CodeInspectionQuickFix> _quickFixes;

    public ImplicitPublicMemberInspectionResult(IInspection inspection, string result, QualifiedContext<ParserRuleContext> qualifiedContext)
        : base(inspection, result, qualifiedContext.ModuleName, qualifiedContext.Context)
    {
        _quickFixes = new CodeInspectionQuickFix[]
        {
            new SpecifyExplicitPublicModifierQuickFix(Context, QualifiedSelection), 
            new IgnoreOnceQuickFix(qualifiedContext.Context, QualifiedSelection, Inspection.AnnotationName), 
        };
    }

    public override IEnumerable<CodeInspectionQuickFix> QuickFixes { get { return _quickFixes; } }
}

All inspection results that can be ignored once have an IgnoreOnceQuickFix; Rubberduck uses this list of quick-fixes to dynamically populate a menu in the code inspection window's toolbar.


Since I introduced the base/abstract class Something doesn't feel right anymore about IInspection: am I right to think it has become superfluous? The base class should simply implement IInspectionModel, and IInspection could be removed - and heck, IInspectionModel could then be renamed back to IInspection as it originally was called. It really feels like CodeInspectionSetting knows more than it needs to about an inspection.

Anything else jumps at you?

\$\endgroup\$
3
\$\begingroup\$

A couple minor suggestions...

I'd introduce some extension methods to give the common predicates on collections of declarations somewhere to live:

public static class DeclarationsPredicates
{
     public static IEnumerable<Declaration> WhichAreUserDeclarations(this IEnumerable<Declaration> source)
     {
         if (source == null)
         {
             // throw an error or sneakily coerce to Enumerable.Empty<Declaration>();
         }

         return source.Where(declaration => !declaration.IsBuiltIn);
     }

     public static IEnumerable<Declaration> WhereInspectionIsNotDisabledByAnnotation(
         this IEnumerable<Declaration> source,
         string annotationName)
     {
         if (source == null)
         {
             // throw an error or sneakily coerce to Enumerable.Empty<Declaration>();
         }

         return source.Where(declaration => !declaration.IsInspectionDisabled(annotationName));
     }
}

I'm sure there are others that you'd find useful! You could now rewrite your properties as:

protected virtual IEnumerable<Declaration> Declarations
{
    get { return State.AllDeclarations.WhereInspectionIsNotDisabledByAnnotation(AnnotationName); }
}

protected virtual IEnumerable<Declaration> UserDeclarations
{
    get { return State.AllUserDeclarations.WhereInspectionIsNotDisabledByAnnotation(AnnotationName); }
}

I think that's better because you've centralised your logic (I'm not overly fond of the name though). You could potentially remove the .IsInspectionDisabled() method from Declaration and put the logic in the extension method but that's up to you.


This is interesting ((dynamic)context.Context)... Why are you doing that?


I agree that SeverityLabel definitely doesn't fit ;) I'd expect it to be readonly anyway:

// C#6
public string SeverityLabel => RubberduckUI.ResourceManager.GetString("CodeInspectionSeverity_" + Severity, RubberduckUI.Culture);

If you want to change the label, you should have to change the severity!


You have a lot of magic strings around the place:

"Inspection", "Meta", "CodeInspectionSeverity_"... They should be well named constants.


The trailing underscore in this name looks odd to me:

RubberduckUI.ImplicitPublicMember_

I'm guessing that the string is "ImplicitPublicMember_"? Either way, chop the underscore off.


This method is named in camelCase but should be PascalCase ambiguousIdentifier()


This property name doesn't look quite right:

 string Meta { get; }

I think it should be Metadata.


Your doc comment here isn't quite right:

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

It actually gets or sets a value...

There's also the <value> tag that you should add. I use the GhostDoc extension to help generate code documentation - that might help you out too.


I agree with your summary at the end - I'd drop back to just using an ICodeInspection interface. I changed the name because I don't like the double I in IInspection and it's also a little more descriptive.

On the whole - very nice!

\$\endgroup\$
  • \$\begingroup\$ Nice answer! The dynamic cast and camelCase methods are ANTLR limitations: the types that expose an ambiguousIdentifier method are generated, and they don't share a common interface. ANTLR parser rules must start with a lowercase, lexer rules with an uppercase - ANTLR generates classes and methods off a grammar definition that must follow this convention. \$\endgroup\$ – Mathieu Guindon Jan 13 '16 at 15:06
  • \$\begingroup\$ As for the trailing underscore, it's a visual cue I'm using to mean "this resource string contains a {0} placeholder that must be formatted". Not sure I'm ready to drop it, although I agree it looks a bit weird. \$\endgroup\$ – Mathieu Guindon Jan 13 '16 at 15:08
  • 1
    \$\begingroup\$ @Mat'sMug - generated code can be so frustrating like that ;) I'd replace that underscore with the word Format \$\endgroup\$ – RobH Jan 13 '16 at 15:13

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.