24
\$\begingroup\$

I've implemented a code inspection that verifies whether a procedure has parameters that are implicitly passed by reference - in VBA a parameter is passed ByRef unless it's explicitly specified ByVal.

Here is the implementation in question:

[ComVisible(false)]
public class ImplicitByRefParameterInspection : IInspection
{
    public ImplicitByRefParameterInspection()
    {
        Severity = CodeInspectionSeverity.Warning;
    }

    public string Name { get { return "Parameter is passed ByRef implicitly"; } }
    public CodeInspectionType InspectionType { get { return CodeInspectionType.CodeQualityIssues; } }
    public CodeInspectionSeverity Severity { get; set; }

    public IEnumerable<CodeInspectionResultBase> GetInspectionResults(SyntaxTreeNode node)
    {
        var procedures = node.FindAllProcedures().Where(procedure => procedure.Parameters.Any(parameter => !string.IsNullOrEmpty(parameter.Instruction.Value)));
        var targets = procedures.Where(procedure => procedure.Parameters.Any(parameter => parameter.IsImplicitByRef)
                                                && !procedure.Instruction.Line.IsMultiline);

        return targets.SelectMany(procedure => procedure.Parameters.Where(parameter => parameter.IsImplicitByRef)
            .Select(parameter => new ImplicitByRefParameterInspectionResult(Name, parameter.Instruction, Severity)));
    }
}

Would there be a better way to do this? Here is the ImplicitByRefParameterInspectionResult class (gosh that's a long name!):

[ComVisible(false)]
public class ImplicitByRefParameterInspectionResult : CodeInspectionResultBase
{
    public ImplicitByRefParameterInspectionResult(string inspection, Instruction instruction, CodeInspectionSeverity type)
        : base(inspection, instruction, type)
    {
    }

    public override IDictionary<string, Action<VBE>> GetQuickFixes()
    {
        return !Handled
            ? new Dictionary<string, Action<VBE>>
                {
                    {"Pass parameter by value", PassParameterByVal},
                    {"Pass parameter by reference explicitly", PassParameterByRef}
                }
            : new Dictionary<string, Action<VBE>>();
    }

    private void PassParameterByRef(VBE vbe)
    {
        if (!Instruction.Line.IsMultiline)
        {
            var newContent = string.Concat(ReservedKeywords.ByRef, " ", Instruction.Value);
            var oldContent = Instruction.Line.Content;

            var result = oldContent.Replace(Instruction.Value, newContent);

            var module = vbe.FindCodeModules(Instruction.Line.ProjectName, Instruction.Line.ComponentName).First();
            module.ReplaceLine(Instruction.Line.StartLineNumber, result);
            Handled = true;
        }
        else
        {
            // todo: implement for multiline
            throw new NotImplementedException("This method is not [yet] implemented for multiline instructions.");
        }
    }

    private void PassParameterByVal(VBE vbe)
    {
        if (!Instruction.Line.IsMultiline)
        {
            var newContent = string.Concat(ReservedKeywords.ByVal, " ", Instruction.Value);
            var oldContent = Instruction.Line.Content;

            var result = oldContent.Replace(Instruction.Value, newContent);

            var module = vbe.FindCodeModules(Instruction.Line.ProjectName, Instruction.Line.ComponentName).First();
            module.ReplaceLine(Instruction.Line.StartLineNumber, result);
            Handled = true;
        }
        else
        {
            // todo: implement for multiline
            throw new NotImplementedException("This method is not yet implemented for multiline instructions.");
        }
    }
}

Any comment is welcome.

\$\endgroup\$
  • \$\begingroup\$ Is IInspection under your control? And is CodeInspectionSeverity Severity { get; set; } part of the interface? \$\endgroup\$ – IEatBagels Dec 18 '14 at 13:31
  • \$\begingroup\$ @TopinFrassi yes, and yes :) \$\endgroup\$ – Mathieu Guindon Dec 18 '14 at 14:36
17
\$\begingroup\$

Starting from this

var procedures = node.FindAllProcedures().Where(procedure => procedure.Parameters.Any(parameter => !string.IsNullOrEmpty(parameter.Instruction.Value)));
var targets = procedures.Where(procedure => procedure.Parameters.Any(parameter => parameter.IsImplicitByRef)
                                        && !procedure.Instruction.Line.IsMultiline);

return targets.SelectMany(procedure => procedure.Parameters.Where(parameter => parameter.IsImplicitByRef)
    .Select(parameter => new ImplicitByRefParameterInspectionResult(Name, parameter.Instruction, Severity)));

Let's collapse it into one expression

return node.FindAllProcedures()
    .Where(procedure => procedure.Parameters.Any(parameter => !string.IsNullOrEmpty(parameter.Instruction.Value)))
    .Where(procedure => procedure.Parameters.Any(parameter => parameter.IsImplicitByRef)
                                        && !procedure.Instruction.Line.IsMultiline)
    .SelectMany(procedure => procedure.Parameters.Where(parameter => parameter.IsImplicitByRef)
    .Select(parameter => new ImplicitByRefParameterInspectionResult(Name, parameter.Instruction, Severity)));

Move the last Select out of SelectMany

return node.FindAllProcedures()
    .Where(procedure => procedure.Parameters.Any(parameter => !string.IsNullOrEmpty(parameter.Instruction.Value)))
    .Where(procedure => procedure.Parameters.Any(parameter => parameter.IsImplicitByRef)
                                        && !procedure.Instruction.Line.IsMultiline)
    .SelectMany(procedure => procedure.Parameters)
    .Where(parameter => parameter.IsImplicitByRef)
    .Select(parameter => new ImplicitByRefParameterInspectionResult(Name, parameter.Instruction, Severity));

Now it seems like procedure.Parameters.Any(parameter => parameter.IsImplicitByRef) is redundant, so let's remove that and join the successive Wheres

return node.FindAllProcedures()
    .Where(procedure => procedure.Parameters.Any(parameter => !string.IsNullOrEmpty(parameter.Instruction.Value))
                        && !procedure.Instruction.Line.IsMultiline)
    .SelectMany(procedure => procedure.Parameters)
    .Where(parameter => parameter.IsImplicitByRef)
    .Select(parameter => new ImplicitByRefParameterInspectionResult(Name, parameter.Instruction, Severity));

It's not clear why procedure.Parameters.Any(parameter => !string.IsNullOrEmpty(parameter.Instruction.Value)) is necessary -- if it is, a comment is required, otherwise we can just write

return node.FindAllProcedures()
    .Where(procedure => !procedure.Instruction.Line.IsMultiline)
    .SelectMany(procedure => procedure.Parameters)
    .Where(parameter => parameter.IsImplicitByRef)
    .Select(parameter => new ImplicitByRefParameterInspectionResult(Name, parameter.Instruction, Severity));

Which is looking more much manageable. Or if you prefer the alternative syntax

return from procedure in node.FindAllProcedures()
       where !procedure.Instruction.Line.IsMultiline
       from parameter in procedure.Parameters
       where parameter.IsImplicitByRef
       select new ImplicitByRefParameterInspectionResult(Name, parameter.Instruction, Severity);
\$\endgroup\$
14
\$\begingroup\$

I don't know how I feel about this ternary.

public override IDictionary<string, Action<VBE>> GetQuickFixes()
{
    return !Handled
        ? new Dictionary<string, Action<VBE>>
            {
                {"Pass parameter by value", PassParameterByVal},
                {"Pass parameter by reference explicitly", PassParameterByRef}
            }
        : new Dictionary<string, Action<VBE>>();
}

I mean, it really could be worse and I admittedly don't like ternaries beyond very simple assignments. I feel like an if statement is called for here. Even though it will add a few lines of code, I think it would add to the readability. On the other hand, for the mess it could have been, it wasn't. Well done. I don't hate it. (Did I mention that I hate ternaries?)

How I would write it.

public override IDictionary<string, Action<VBE>> GetQuickFixes()
{
    var result = new Dictionary<string, Action<VBE>>;

    if (!Handled)
    {
        result.Add("Pass parameter by value", PassParameterByVal);
        result.Add("Pass parameter by reference explicitly", PassParameterByRef);
    }

    return result;
}

There are pros and cons to each method. My way reduces the level of indentation, but introduces an intermediate variable

Hmm.. that ternary is in every implementation.... – Mat's Mug

I think GetQuickFixes in the base class should take a dictionary in as an argument. Then this logic doesn't need to be in every implementation. It just needs to create the proper dict for the unhandled case, pass it to base, and return whatever dictionary the base deems necessary. That way this logic doesn't need to be implemented over and over again. You should end up with something that looks like this.

public override IDictionary<string, Action<VBE>> GetQuickFixes()
{
    var fixes = new Dictionary<string, Action<VBE>>
         {
             {"Pass parameter by value", PassParameterByVal},
             {"Pass parameter by reference explicitly", PassParameterByRef}
         } 
    return base.GetQuickFixes(fixes);
}

And now the ternary becomes acceptable in the base class.

public IDictionary<string, Action<VBE>> GetQuickFixes(IDictionary<string, Action<VBE>> actions)
{
    return !Handled ? actions : new Dictionary<string, Action<VBE>>();
}
\$\endgroup\$
  • \$\begingroup\$ Hmm.. that ternary is in every implementation.... \$\endgroup\$ – Mathieu Guindon Dec 18 '14 at 0:25
  • 1
    \$\begingroup\$ It's honestly not that bad. Just a pet peeve and I'm sure for every person who agrees with me, there will be two who don't. \$\endgroup\$ – RubberDuck Dec 18 '14 at 0:26
  • \$\begingroup\$ Personally i would rearrange the ternary but keep it as a ternary, because you know it's a hammer. \$\endgroup\$ – Malachi Dec 18 '14 at 4:16
  • 1
    \$\begingroup\$ I like this. It creates the return value only once. \$\endgroup\$ – Mathieu Guindon Dec 18 '14 at 4:27
  • \$\begingroup\$ Okay. Last edit I think. Had another idea. It bothered me that the same logic was in every implementation. \$\endgroup\$ – RubberDuck Dec 18 '14 at 4:59
12
\$\begingroup\$
public override IDictionary<string, Action<VBE>> GetQuickFixes()
{
    return !Handled
        ? new Dictionary<string, Action<VBE>>
            {
                {"Pass parameter by value", PassParameterByVal},
                {"Pass parameter by reference explicitly", PassParameterByRef}
            }
        : new Dictionary<string, Action<VBE>>();
}

With this I would turn this around, so that I could use Handled instead of not handled...

so it would look something like this

public override IDictionary<string, Action<VBE>> GetQuickFixes()
{
    return Handled
        ? new Dictionary<string, Action<VBE>>()
        : new Dictionary<string, Action<VBE>>
            {
                {"Pass parameter by value", PassParameterByVal},
                {"Pass parameter by reference explicitly", PassParameterByRef}
            };
}
\$\endgroup\$
11
\$\begingroup\$
var newContent = string.Concat(ReservedKeywords.ByVal, " ", Instruction.Value);

Here's a nifty thing I learned recently:

var newContent = ReservedKeywords.ByVal + " " + Instruction.Value;

will be optimized by the compiler to a call to string.Concat, exactly the same as above. It's also more readable, so you have nothing to lose. You can read more about it on Eric Lippert's blog.


The methods PassParameterByRef and PassParameterByVal are identical except for one variable, and could (unless you're planning on changing them) just call in to a common method.

\$\endgroup\$
6
\$\begingroup\$

I'm not sure about the fact that Severity is public set. As a client of your code, who am I to say that ImplicitByRefParameterInspection has a severity of X when you, as the dev, who knows his stuff, said it had a severity of Warning.

I think your interface should expose only the getter on CodeInspectionSeverity Severity.

I'm not part of the project but I have a hard time to see a scenario where you would want to set the severity of an IInspection. Since it is an abstraction, you can't tell how severe it is.

(I think) You want to set the severity of an implementation of IInspection.

If I turn this into code, it would look like this :

public interface IInspection
{
    CodeInspectionSeverity Severity { get; }
}

[ComVisible(false)]
public class ImplicitByRefParameterInspection : IInspection
{
    public ImplicitByRefParameterInspection()
    {
        Severity = CodeInspectionSeverity.Warning;
    }

    public CodeInspectionSeverity Severity { get; private set; }
}

This way, I (as a client) can know the severity of an inspection, but I can't set it, and it is a good thing, since I have absolutely no clue what it should be.

\$\endgroup\$
  • 1
    \$\begingroup\$ I should have included the enum in the post.. it goes from DoNotShow to Error - it's intended to be configurable by the dev using it - if you want to show an inspection type as a "hint", as a "suggestion" or as a "warning", or if you just don't want to see it, you can configure that. The user (VBA/COM) doesn't see that type, and cannot access it programmatically. \$\endgroup\$ – Mathieu Guindon Dec 18 '14 at 15:30
  • 1
    \$\begingroup\$ +1 Because your answer makes sense given the context, but in fact there is a configuration that lets the user specify what the severity level should be]. \$\endgroup\$ – RubberDuck Dec 18 '14 at 15:30
  • \$\begingroup\$ I wasn't sure at the moment I wrote my answer, but I thought it could be useful anyway! :) \$\endgroup\$ – IEatBagels Dec 18 '14 at 15:33

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.