HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Of Procedures and Variables: never enough nodes

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
proceduresnevernodesvariablesandenough

Problem

Building on my ANTLR tree listener, I'm now starting to see how the whole thing is coming together.

As I proceed to implement the numerous Node classes I'm going to need to make sense out of the ANTLR parse tree, I feel like I'm doing more and more weird things.. oh, the tests pass. But the implementations... here, have a look:

ProcedureNode

Used for Sub, Function, Property Get, Property Let and Property Set code blocks, this node can have children and defines a scope.

```
public class ProcedureNode : Node
{
public enum VBProcedureKind
{
Sub,
Function,
PropertyGet,
PropertyLet,
PropertySet
}

public ProcedureNode(VisualBasic6Parser.PropertySetStmtContext context, string scope, string localScope)
: this(context, scope, localScope, VBProcedureKind.PropertySet, context.visibility(), context.ambiguousIdentifier(), null)
{
}

public ProcedureNode(VisualBasic6Parser.PropertyLetStmtContext context, string scope, string localScope)
: this(context, scope, localScope, VBProcedureKind.PropertyLet, context.visibility(), context.ambiguousIdentifier(), null)
{
}

public ProcedureNode(VisualBasic6Parser.PropertyGetStmtContext context, string scope, string localScope)
: this(context, scope, localScope, VBProcedureKind.PropertyGet, context.visibility(), context.ambiguousIdentifier(), context.asTypeClause)
{
}

public ProcedureNode(VisualBasic6Parser.FunctionStmtContext context, string scope, string localScope)
: this(context, scope, localScope, VBProcedureKind.Function, context.visibility(), context.ambiguousIdentifier(), context.asTypeClause)
{
}

public ProcedureNode(VisualBasic6Parser.SubStmtContext context, string scope, string localScope)
: this(context, scope, localScope, VBProcedureKind.Sub, context.visibility(), context.ambiguousIdentifier(), null)
{
}

private ProcedureNode(ParserRuleContext context, stri

Solution

ProcedureNode constructor

The private constructor can be simplified / made be more readable by returning early if asType == null. You also shouldn't check for returnTypeClause == null twice.

You could consider to change the private readonly string _returnType; to a "normal" string and add a private setter to the property. By doing this you could get rid of the tenary expression by setting _returnType = ReservedKeywords.Variant at initialization.

private ProcedureNode(ParserRuleContext context, string scope, string localScope, 
                      VBProcedureKind kind, 
                      VisualBasic6Parser.VisibilityContext visibility, 
                      VisualBasic6Parser.AmbiguousIdentifierContext name, 
                      Func asType)
    : base(context, scope, localScope)
{
    _kind = kind;
    _name = name.GetText();
    _accessibility = visibility.GetAccessibility();

    if (asType == null) { return; }

    var returnTypeClause = asType();

    var returnTypeClauseIsNull  = returnTypeClause == null;

    _isImplicitReturnType = returnTypeClauseIsNull;

    _returnType = returnTypeClauseIsNull   
                    ? ReservedKeywords.Variant 
                    : returnTypeClause.type().GetText();
}


ParserRuleContextExtensions

With the given name of the class, the GetAccessibility() method doesn't belong there. If you consider to keep both methods in this class, you should consider to rename the inputparameters to parserRuleContext and visibilityContext.

VariableNode constructor

After reordering the assignments in the way _accessibility is assigned at the top you can reduce horizontal spacing by using guard clauses.

Storing the result of context.asTypeClause() and context.typeHint() will reduce code duplication.

public VariableNode(VisualBasic6Parser.VariableSubStmtContext context, string scope, 
                    VisualBasic6Parser.VisibilityContext visibility, bool isLocal = true)
    : base(context, scope)
{
    _name = context.ambiguousIdentifier().GetText();
    _accessibility = isLocal ? VBAccessibility.Private : visibility.GetAccessibility();

    var asTypeClause = context.asTypeClause();
    if (asTypeClause != null)
    {
        _typeName = asTypeClause.type().GetText();
        return;
    }

    var hint = context.typeHint();
    if (hint  == null)
    {
        _isImplicitlyTyped = true;
        _typeName = ReservedKeywords.Variant;
        return;
    }

    _isUsingTypeHint = true;
    _typeName = TypeSpecifiers[hint.GetText()];
}

Code Snippets

private ProcedureNode(ParserRuleContext context, string scope, string localScope, 
                      VBProcedureKind kind, 
                      VisualBasic6Parser.VisibilityContext visibility, 
                      VisualBasic6Parser.AmbiguousIdentifierContext name, 
                      Func<VisualBasic6Parser.AsTypeClauseContext> asType)
    : base(context, scope, localScope)
{
    _kind = kind;
    _name = name.GetText();
    _accessibility = visibility.GetAccessibility();

    if (asType == null) { return; }

    var returnTypeClause = asType();

    var returnTypeClauseIsNull  = returnTypeClause == null;

    _isImplicitReturnType = returnTypeClauseIsNull;

    _returnType = returnTypeClauseIsNull   
                    ? ReservedKeywords.Variant 
                    : returnTypeClause.type().GetText();
}
public VariableNode(VisualBasic6Parser.VariableSubStmtContext context, string scope, 
                    VisualBasic6Parser.VisibilityContext visibility, bool isLocal = true)
    : base(context, scope)
{
    _name = context.ambiguousIdentifier().GetText();
    _accessibility = isLocal ? VBAccessibility.Private : visibility.GetAccessibility();

    var asTypeClause = context.asTypeClause();
    if (asTypeClause != null)
    {
        _typeName = asTypeClause.type().GetText();
        return;
    }

    var hint = context.typeHint();
    if (hint  == null)
    {
        _isImplicitlyTyped = true;
        _typeName = ReservedKeywords.Variant;
        return;
    }

    _isUsingTypeHint = true;
    _typeName = TypeSpecifiers[hint.GetText()];
}

Context

StackExchange Code Review Q#78583, answer score: 5

Revisions (0)

No revisions yet.