Code Inspection: Procedure not used

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 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 handlers)
{
var result =
declaration.DeclarationType != DeclarationType.Procedure
|| handlers.Contains(declaration)
|| declaration.References.Any()
|| IsPublicModuleMember(declarations, declaration)
|| IsClassLifeCycleHandler(declarations, declaration)

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

    /// 
    /// 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".
    /// 
    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?

StackExchange Code Review Q#85881, answer score: 2