Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

As part of a project I'm working on, I've implemented a class that encapsulates the syntax for a / Dim statement.

Given a line of code, the Parse method uses a regular expression to match the keyword's syntax and extract the relevant parts.

As it stands there's no support for some valid syntaxes, the following aren't currently supported:

'multiple declarations on the same line:
Dim foo As String, bar As String 

'multiple instructions on the same line:
Dim foo As String: Dim bar As String 

But this question isn't about the code that's not yet implemented, it's about the approach taken; the regular expression works (for the so-far supported use cases), but it's quickly going to become a problem (read: mess), especially since DimSyntax isn't going to be the only ISyntax implementation in the code.

On the other hand, isn't likely to ever change in the future, so perhaps a well-crafted regex can do the job, if it won't need to be maintained?

public class DimSyntax : ISyntax
{
    // todo: ditch regex
    private static readonly string _syntaxPattern = 
        @"^((?<keyword>Dim)\s(?<name>\w{1,30})\s?(As\s(?<type>\w{1,30})))?";

    private static readonly Regex _regex = new Regex(_syntaxPattern);

    private readonly IVBTypeFactory _typeFactory;

    public DimSyntax(IVBTypeFactory typeFactory, IIdentifier identifier)
    {
        _typeFactory = typeFactory;
        _identifier = identifier;
    }

    private readonly IIdentifier _identifier;
    public IIdentifier Identifier { get { return _identifier; } }

    public string Keyword { get { return Keywords.Dim; } }

    public ISyntax Parse(string line)
    {
        var code = line.Trim();

        var match = _regex.Match(code);
        if (!match.Success)
        {
            return null;
        }

        var nameGroup = match.Groups["name"];
        var typeGroup = match.Groups["type"];
        if (nameGroup == null || typeGroup == null)
        {
            return null;
        }

        var type = _typeFactory.Parse(nameGroup.Value);
        var identifier = new Identifier(nameGroup.Value, type);

        return new DimSyntax(_typeFactory, identifier);
    }

    public override string ToString()
    {
        if (_identifier.Type != null)
        {
            // explicit type
            return string.Format("{0} {1} {2} {3}", Keyword, _identifier.Name, 
                                                  Keywords.As, _identifier.Type.TypeName);
        }
        else
        {
            // implicit type - Variant
            return string.Format("{0} {1}", Keyword, _identifier.Name);
        }
    }
}

Keywords is just a .resx resource file that contains a list of / keywords. I was going to make a public static class with a bunch of public static readonly string members, but I find it's cleaner to use a .resx file. Other suggestions?

I have mixed feelings about the IVBTypeFactory being a dependency of the type, when it's really a dependency of the Parse method - would it make sense to just take the abstract factory in as a method parameter instead?


For reference & context, here's the Identifier implementation:

public class Identifier : IIdentifier
{
    public Identifier(string name, IVBType type)
    {
        _name = name;
        _type = type;
    }

    private readonly string _name;
    public string Name { get { return _name; } }

    private readonly IVBType _type;
    public IVBType Type { get { return _type; } }
}

Pretty much all types are immutable, which means any number of threads can safely access the parsed ISyntax objects - am I making my life harder than it needs to be here?

If you need more context, or are curious to see more, the rest of the code can be found on GitHub.

share|improve this question
    
Just realized, I had the wrong definition for both parser and lexer... –  Mat's Mug Jun 9 at 16:43

3 Answers 3

Finally, a question where we can use Roslyn!

I won't review your own code but instead I whipped up this small showcase of how you might do it with the Roslyn compilers. Do note that it is rather ugly, not in the least because I am not entirely comfortable with it myself yet but also because I know little VB.NET and it appears some things are handled differently from the C# compiler. That being said, this sample implementation can parse both

Dim foo As String

and

Dim foo As String, bar As String 

El codos:

using System;
using System.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.VisualBasic.Syntax;
using Microsoft.CodeAnalysis.VisualBasic;

class Program
{
    static void Main(string[] args)
    {
        var stuffIWantToParse = "Dim foo As String, bar As String";

        var tree = VisualBasicSyntaxTree.ParseText(@"
            Module Module1
                 " + stuffIWantToParse +
            @" End Module
        ");

        var compilation = VisualBasicCompilation.Create("MyCompilation", syntaxTrees: new[] { tree });
        var moduleType = compilation.GetTypeByMetadataName("Module1");

        PrintVariableDeclarationByIndex(0, moduleType);
        PrintVariableDeclarationByIndex(1, moduleType);

        Console.Read();
    }

    private static void PrintVariableDeclarationByIndex(int i, INamedTypeSymbol moduleType)
    {
        var variable = moduleType.GetMembers().Skip(i).FirstOrDefault();

        var position = variable.Locations.First().SourceSpan.Start;
        var root = variable.Locations.First().SourceTree.GetRoot();
        var parent = root.FindToken(position).Parent;
        var node = parent.FirstAncestorOrSelf<FieldDeclarationSyntax>();
        var typeInfo = node.Declarators[i].AsClause;

        Console.WriteLine("Modifiers: " + string.Join(",", node.Modifiers));
        Console.WriteLine("Name: " + variable.Name);
        Console.WriteLine("Type: " + typeInfo.Type());
        Console.WriteLine();
    }
}

Output:

enter image description here

Using a workaround from here. If you want some more codesamples on Roslyn, look at my project here. In order to use the Visual Basic compilers look here. For more information on Roslyn, take a look at the tag wiki I put together.

share|improve this answer
1  
That's... rather interesting! Does Microsoft.CodeAnalysis.VisualBasic work with VB6/VBA or just VB.NET? –  Mat's Mug Jun 9 at 17:01
    
Aww.. It's just for VB.NET I believe. Are there difference between the As statements between these versions? –  Jeroen Vannevel Jun 9 at 17:03
    
Well in theory VB6 could be parsed as VB.NET (I think), ..it might be worth a serious shot. –  Mat's Mug Jun 9 at 17:09

You appear to be missing something in your object model. Consider this snippet that creates the regex pattern.

private static readonly string _syntaxPattern = 
    @"^((?<keyword>Dim)\s(?<name>\w{1,30})\s?(As\s(?<type>\w{1,30})))?";

And this snippet that defines the keyword for the class.

public string Keyword { get { return Keywords.Dim; } }

If you stick with the regex to do this, you'll find that you can use an almost identical pattern for several ISyntax implementations. For a hypothetical PublicSyntax you would have this:

private static readonly string _syntaxPattern = 
    @"^((?<keyword>Public)\s(?<name>\w{1,30})\s?(As\s(?<type>\w{1,30})))?";

and

public string Keyword { get { return Keywords.Public; } }

This tells me that both of those need to inherit a GetSyntaxPattern() function that returns something like this.

protected virtual string GetSyntaxPattern()
{
    return "^((?" + this.Keyword + ")\s(?<name>\w{1,30})\s?(As\s(?<type>\w{1,30})))?";
}

Or perhaps it would be even better as a property.

protected virtual string SyntaxPattern()
{
    get { return "^((?" + this.Keyword + ")\s(?<name>\w{1,30})\s?(As\s(?<type>\w{1,30})))?"; }
}
share|improve this answer

If you're actually going to need to parse more than just Dims, I think the right solution is to use a parser generator like ANTLR. That way, the syntax is going to be more readable than a bunch of overcomplicated regexes, the code is going to be more DRY (because you won't need to write all the group-extraction code) and the result is going to be a proper syntax tree.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.