Convert Procedure to Function

My latest inspection for Rubberduck determines whether a procedure should be a function by checking whether it has a single ByRef parameter (either explicit or implicit). The quick fix takes this procedure, changes it into a function, and updates all calls.

This inspection starts deep down, involving the parser:

private class ArgListWithOneByRefParamListener : VBABaseListener
{
    private readonly IList _contexts = new List();
    public IEnumerable Contexts { get { return _contexts; } }

    public override void ExitArgList(VBAParser.ArgListContext context)
    {
        if (context.arg() != null && context.arg().Count(a => a.BYREF() != null || (a.BYREF() == null && a.BYVAL() == null)) == 1)
        {
            _contexts.Add(context);
        }
    }
}

Here, we have a method that listens for any ArgLists and determines whether they have a single ByRef parameter. If so, it adds the ArgListContext to a list exposed through the parser state class.

Here is the inspection:

```
public class ProcedureShouldBeFunctionInspection : IInspection
{
public ProcedureShouldBeFunctionInspection()
{
Severity = CodeInspectionSeverity.Warning;
}

public string Name { get { return "ProcedureShouldBeFunctionInspection"; } }
public string Meta { get { return InspectionsUI.ResourceManager.GetString(Name + "Meta"); } }
public string Description { get { return InspectionsUI.ProcedureShouldBeFunctionInspection; } }
public CodeInspectionType InspectionType { get { return CodeInspectionType.LanguageOpportunities; } }
public CodeInspectionSeverity Severity { get; set; }

public IEnumerable GetInspectionResults(RubberduckParserState state)
{
return state.ArgListsWithOneByRefParam
.Where(context => context.Context.Parent is VBAParser.SubStmtContext)
.Select(context => new ProcedureShouldBeFunctionInspectionResult(this,
state,
new QualifiedContext(context.Module

-
This condition a.BYREF() != null || (a.BYREF() == null && a.BYVAL() == null) can be simplified to a.BYREF() != null || a.BYVAL() == null since if BYREF is not unequal to null then it is equal to null.

Also this logic is repeated again in another class - this violates DRY. It should be a method on the ArgContext (if I got the type right).

-
In ProcedureShouldBeFunctionInspection some properties reference the InspectionUI - it seems odd that an engine style class has a reference to the UI but there might be perfectly good reasons for it. Just seems a bit suspicious so I mentioned it.

-
In GetInspectionResults you use as in several places. Would it make sense for those casts to return null? Can the constructed objects deal with those parameters being null? If not then you should use a direct cast instead.

-
This:

_lineOffset = newfunctionWithReturn.Split(new[] {Environment.NewLine}, StringSplitOptions.None).Length -
             subStmtText.Split(new[] {Environment.NewLine}, StringSplitOptions.None).Length;

seems sufficiently ugly to warrant a string extension method (especially since I would suspect you might do this in other parts of your project):

public static IList SplitLines(this string s)
{
    return s.Split(new[] { Environment.NewLine }, StringSplitOptions.None);
}

which would translate the above code into:

_lineOffset = newfunctionWithReturn.SplitLines().Count -
             subStmtText.SplitLines().Count;

-
Extracting sub expressions with multiple uses into a local variable can help to improve readability. Like this:

var startLine = _subStmtQualifiedContext.Context.Start.Line;
var stopLine =  _subStmtQualifiedContext.Context.Stop.Line;

module.DeleteLines(startLine, stopLine - startLine + 1);
module.InsertLines(startLine, newfunctionWithReturn);

-
referenceParent.argsCall().argCall().ToList().ElementAt(_argListQualifiedContext.Context.arg().ToList().IndexOf(_argQualifiedContext.Context)).GetText() - Seriously?

-
I think I've mentioned it in a previous review, but I'll repeat it again: You have call chains 3 or 4 items deep on a regular basis - this indicates that the abstractions are probably not quite right.

StackExchange Code Review Q#115477, answer score: 2