Code Inspection: Procedure not used

Question

Here is yet another piece of Rubberduck code, this time the nasty ProcedureNotUsedInspection class, whose role is to identify all procedures that are never called anywhere, and to issue a ProcedureNotUsedInspectionResult for each one.

using System.Collections.Generic;
using System.Linq;
using Rubberduck.Parsing;
using Rubberduck.Parsing.Grammar;
using Rubberduck.Parsing.Symbols;

namespace Rubberduck.Inspections
{
    public class ProcedureNotUsedInspection : IInspection
    {
        public ProcedureNotUsedInspection()
        {
            Severity = CodeInspectionSeverity.Hint;
        }

        public string Name { get { return InspectionNames.ProcedureNotUsed_; } }
        public CodeInspectionType InspectionType { get { return CodeInspectionType.CodeQualityIssues; } }
        public CodeInspectionSeverity Severity { get; set; }

        public IEnumerable<CodeInspectionResultBase> GetInspectionResults(VBProjectParseResult parseResult)
        {
            var handlers = parseResult.Declarations.Items.Where(item => item.DeclarationType == DeclarationType.Control)
                .SelectMany(control => parseResult.Declarations.FindEventHandlers(control));

            var issues = parseResult.Declarations.Items
                .Where(item => !IsIgnoredProcedure(parseResult.Declarations, item, handlers))
                .Select(issue => new IdentifierNotUsedInspectionResult(string.Format(Name, issue.IdentifierName), Severity, issue.Context, issue.QualifiedName.QualifiedModuleName));

            return issues;
        }

        private bool IsIgnoredProcedure(Declarations declarations, Declaration declaration, IEnumerable<Declaration> handlers)
        {
            var result = 
                declaration.DeclarationType != DeclarationType.Procedure
                || handlers.Contains(declaration)
                || declaration.References.Any()
                || IsPublicModuleMember(declarations, declaration)
                || IsClassLifeCycleHandler(declarations, declaration)
                || IsInterfaceMember(declarations, declaration);

            return result;
        }

        /// <remarks>
        /// We cannot determine whether exposed members of standard modules are called or not,
        /// so we assume they are instead of flagging them as "never called".
        /// </remarks>
        private bool IsPublicModuleMember(Declarations declarations, Declaration procedure)
        {
            var parent = declarations.Items.SingleOrDefault(item =>
                        item.IdentifierName == procedure.ComponentName &&
                        (item.DeclarationType == DeclarationType.Module));

            return parent != null && (procedure.Accessibility == Accessibility.Implicit
                                      || procedure.Accessibility == Accessibility.Public);
        }

        private static readonly string[] ClassLifeCycleHandlers =
        {
            "Class_Initialize",
            "Class_Terminate"
        };

        private bool IsClassLifeCycleHandler(Declarations declarations, Declaration procedure)
        {
            var parent = declarations.Items.SingleOrDefault(item =>
                        item.IdentifierName == procedure.ComponentName &&
                        (item.DeclarationType == DeclarationType.Class));

            return parent != null && ClassLifeCycleHandlers.Contains(procedure.IdentifierName);
        }

        /// <remarks>
        /// Interface implementation members are private, they're not called from an object
        /// variable reference of the type of the procedure's class, and whether they're called or not,
        /// they have to be implemented anyway, so removing them would break the code.
        /// Best just ignore them.
        /// </remarks>
        private bool IsInterfaceMember(Declarations declarations, Declaration procedure)
        {
            // get the procedure's parent module
            var parent = declarations.Items.SingleOrDefault(item =>
                       item.IdentifierName == procedure.ComponentName &&
                       (item.DeclarationType == DeclarationType.Class));

            if (parent == null)
            {
                return false;
            }

            var classes = declarations.Items.Where(item => item.DeclarationType == DeclarationType.Class);
            var interfaces = classes.Where(item => item.References.Any(reference =>
                    reference.Context.Parent is VBAParser.ImplementsStmtContext));

            if (interfaces.Select(i => i.ComponentName).Contains(procedure.ComponentName))
            {
                return true;
            }

            // todo: find a way to avoid running this for every procedure in a class
            var result = GetImplementedInterfaceMembers(declarations, procedure.ComponentName)
                .Contains(procedure.IdentifierName);

            return result;
        }

        private IEnumerable<string> GetImplementedInterfaceMembers(Declarations declarations, string componentName)
        {
            var classes = declarations.Items.Where(item => item.DeclarationType == DeclarationType.Class);
            var interfaces = classes.Where(item => item.References.Any(reference =>
                    reference.Context.Parent is VBAParser.ImplementsStmtContext
                    && reference.QualifiedModuleName.ModuleName == componentName));

            var members = interfaces.SelectMany(declarations.FindMembers)
                .Select(member => member.ComponentName + "_" + member.IdentifierName);
            return members;
        }
    }
}

As you can see there are a number of constraints and assumptions that we need to make here:

• Any Public procedure that can be called from Excel as a user-defined function or a "macro" is ignored, because we can't tell whether it's actually used or not.
• Event handlers and class life cycle handlers are called by VBA itself, so they're ignored too.
• An interface can only be recognized as such if it's implemented somewhere.

Now, the main issue I have is one of performance - I need to figure out a way to reorganize the code in this inspection so as to avoid doing redundant checks like I'm doing; I'd like this code to be as efficient as possible, and I know it isn't - there are redundancies with detecting interface members, and I'm "finding" interface members way too many times (a module's members aren't going to change between two iterations!).

How can I clean this up without breaking it?

Answer 1

Some basic refactoring:

Make members static

you can make IsIgnoredProcedure(), IsPublicModuleMember(), IsClassLifeCycleHandler(), IsInterfaceMember() and GetImplementedInterfaceMembers() static since they do not use any instance info. Alternatively, if each instance of ProcedureNotUsedInspection were to be immutable (i.e. determine the results at instantiation and generate a new one every time code changes) then you could make the declarations code a field.

Making the methods static isn't really a great refactoring but I find that when something is static the compiler helps me to think about when I am working with instance level or common level things.

Extract HasParent

I created a method called HasParent() since IsPublicModuleMember(), IsClassLifeCycleHandler() and IsInterfaceMember() run the same code to determine if the declaration has a parent (as far as I can see). This simplifies those methods also because there is no need to pass in the declarations.

    private static bool IsIgnoredProcedure(Declarations declarations, Declaration declaration, IEnumerable<Declaration> handlers)
    {
        var result = !ProcedureTypes.Contains(declaration.DeclarationType)
            || declaration.References.Any()
            || handlers.Contains(declaration)
            || HasParent(declarations, declaration)
            || IsPublicModuleMember(declaration)
            || IsClassLifeCycleHandler(declaration)
            || IsInterfaceMember(declarations, declaration);

        return result;
    }

    private static bool HasParent(Declarations declarations, Declaration procedure)
    {
        var parent = declarations.Items.SingleOrDefault(item =>
                     item.Project == procedure.Project &&
                     item.IdentifierName == procedure.ComponentName &&
                     item.DeclarationType == DeclarationType.Module);

        return parent != null;
    }

    /// <remarks>
    /// We cannot determine whether exposed members of standard modules are called or not,
    /// so we assume they are instead of flagging them as "never called".
    /// </remarks>
    private static bool IsPublicModuleMember(Declaration procedure)
    {
        return procedure.Accessibility == Accessibility.Implicit
            || procedure.Accessibility == Accessibility.Public;
    }

    private static readonly string[] ClassLifeCycleHandlers =
    {
        "Class_Initialize",
        "Class_Terminate"
    };

    private static bool IsClassLifeCycleHandler(Declaration procedure)
    {
        return ClassLifeCycleHandlers.Contains(procedure.IdentifierName);
    }

Recommend extract methods

I would also suggest that you create some method (or access a property) that provides the classes and interfaces that are declared. Then you can call them from IsInterfaceMember(). Surely these functions will be pretty commonly used, so they could be located elsewhere.

Question

I haven't used interfaces a lot in VBA, its a little cumbersome IMO. So I'm not completely confident in this, but I have a question about the following in IsInterfaceMember():

        if (interfaces.Select(i => i.ComponentName).Contains(procedure.ComponentName))
        {
            return true;
        }

This checks to see if there are any interface names in the name of the procedure, trying to match Interface_Procedurename. But surely you would need to ensure that the declaration is inside a class and that class implements the interface (or can modules implement interfaces too)?

I think that you will need to separate the declarations into separate groups and work with them for the specific cases. Can you work with classes/modules and then inspect their declarations?