Look 'ma, I can read code

Question

I have put together a naive implementation of a VB6/VBA parser, and I'd like to see if the CR community sees the same things as I see can be improved with this code, before I start refactoring.

I've never written a parser, so this naive implementation is naive in all senses: I wanted to push it as far as I could possibly go, until I stopped to refactor - so here I am with a CodeFileParser that does everything except recursively parse code blocks; the actual language syntax isn't yet processed at this point, but the code still produces an ISyntaxTree that models the entire code file.

public interface ISyntaxTree
{
    string Name { get; }
    IEnumerable<IAttribute> Attributes { get; }
    IList<ISyntaxTree> Nodes { get; }
}

For example I can give it a List class (here modified to implement some IEnumerable interface), and with a little HierarchicalDataTemplate in a sandbox WPF app (which probably could also use a peer review!), I get this.. which is already awesome:

So when we parse Public Property Get Item(index As Long) As Variant, it's broken down like this:

• PropertyNode ("Item (Get)")
  • IdentifierNode ("Item")
    • ReferenceNode ("Variant")
  • ParameterNode ("index")
    • IdentifierNode ("index")
      • ReferenceNode ("Long")
  • CodeBlockNode
    • (more CodeBlockNodes)

So an IdentifierNode basically represents an identifier; I'm self-debating whether an IdentifierNode should have a Scope property, or if an identifier's scope should be inferred from its position in the tree. Anything that has a name, should have an IdentifierNode to store it: method names, class names, variables, constants, everything.

An identifier that has an explicit type specified, will have a ReferenceNode under it. A ReferenceNode represents a usage of a type, either built-in (String, Integer, etc.) or pointing to an IdentifierNode's Name. For functions and property getters (i.e. members with a return value), the ReferenceNode under the member's IdentifierNode points to the return type.

The idea is to have enough metadata in the tree structure, to be able to eventually write code inspectors that can walk one or more such trees and locate all references of a given identifier, for example - and then perhaps apply a rename refactoring. Or find all unused variables. Whatever. Before I can start analyzing the tree structure, I need to refactor this piece of crap first:

Parse

public class CodeFileParser
{
    public ISyntaxTree Parse (string fileName)
    {
        var content = File.ReadAllLines(fileName);
        var currentLine = 0;

        var header = ParseFileHeader(fileName, content, ref currentLine);
        var declarations = ParseDeclarations(content, ref currentLine);
        var members = ParseMembers(content, ref currentLine);

        var module = new ModuleNode(header, declarations, members);
        return module;
    }

ParseFileHeader

I don't like the hard-coding going on here, the function shouldn't "know" the contents of every line in the file header... Also AttributeParser is a relic from a previous approach, which might not be crazy at all (but can certainly be cleaned up):

    private ISyntaxTree ParseFileHeader(string fileName, string[] content, ref int currentLine)
    {
        var attributeParser = new AttributeParser();
        IList<IAttribute> attributes = new List<IAttribute>();
        ISyntaxTree result;

        var firstLine = content[0].Trim();
        if (firstLine == "VERSION 1.0 CLASS")
        {
            var multiUse = content[2].Trim();
            var persistable = content[3].Trim();
            var dataBindingBehavior = content[4].Trim();
            var dataSourceBehavior = content[5].Trim();
            var mtsTransactionMode = content[6].Trim();

            attributes.Add(attributeParser.Parse(content[8].Trim()));
            attributes.Add(attributeParser.Parse(content[9].Trim()));
            attributes.Add(attributeParser.Parse(content[10].Trim()));
            attributes.Add(attributeParser.Parse(content[11].Trim()));
            attributes.Add(attributeParser.Parse(content[12].Trim()));

            var regex = new Regex(@"\=\s\-?(?<IntValue>\d+)\s");
            result = new ClassModule(fileName, attributes)
                            {
                                DataBindingBehavior = int.Parse(regex.Match(dataBindingBehavior).Groups["IntValue"].Value),
                                DataSourceBehavior = int.Parse(regex.Match(dataSourceBehavior).Groups["IntValue"].Value),
                                MTSTransactionMode = int.Parse(regex.Match(mtsTransactionMode).Groups["IntValue"].Value),
                                MultiUse = int.Parse(regex.Match(multiUse).Groups["IntValue"].Value),
                                Persistable = int.Parse(regex.Match(persistable).Groups["IntValue"].Value)
                            };

            currentLine = 13;
        }
        else
        {
            attributes.Add(attributeParser.Parse(content[0].Trim()));
            result = new CodeModule(fileName, attributes);

            currentLine = 1;
        }

        return result;
    }

ParseDeclarations

    private IEnumerable<ISyntaxTree> ParseDeclarations(string[] content, ref int currentLine)
    {
        var result = new List<ISyntaxTree>();

        var pattern = @"((?<keyword>Dim|Static|Public|Private|Friend|Global)\s)?(?<keyword>Dim|Static|Public|Private|Friend|Global|Const|Declare|Type|Enum)\s+(?<identifier>\w+)(?<arraySize>\(.*\))?(\s+As\s+?(((?<initializer>New)\s+)?)(?<type>\w+(\.\w+)?))?$";
        var regex = new Regex(pattern);

        var isDeclarationSection = true;
        while (isDeclarationSection)
        {
            if (content[currentLine].Trim().StartsWith("'"))
            {
                // comment node
                currentLine++;
                continue;
            }

            var line = content[currentLine];
            isDeclarationSection =  !(
                                           line.Contains(ReservedKeywords.Property)
                                        || line.Contains(ReservedKeywords.Sub)
                                        || line.Contains(ReservedKeywords.Function)
                                     );
            if (isDeclarationSection)
            {
                currentLine++;
                var match = regex.Match(line);
                if (match.Success && !line.StartsWith(ReservedKeywords.Implements))
                {
                    if (match.Groups["keyword"].Captures.Count == 2)
                    {
                        result.Add(new DeclarationNode(match.Groups["keyword"].Captures[0].Value, 
                                                       match.Groups["keyword"].Captures[1].Value,
                                                       match.Groups["identifier"].Value,
                                                       match.Groups["arraySize"].Value,
                                                       match.Groups["initializer"].Value,
                                                       match.Groups["type"].Value));
                    }
                    else
                    {
                        result.Add(new DeclarationNode(null, match.Groups["keyword"].Value,
                                                             match.Groups["identifier"].Value,
                                                             match.Groups["arraySize"].Value,
                                                             match.Groups["initializer"].Value,
                                                             match.Groups["type"].Value));
                    }
                }
                else if(line.StartsWith(ReservedKeywords.Implements))
                {
                    var implements = Regex.Match(line, ReservedKeywords.Implements + @"\s(?<type>[a-zA-Z][_a-zA-Z0-9]*)$");
                    if (implements.Success)
                    {
                        var reference = new ReferenceNode(implements.Groups["type"].Value);
                        result.Add(new InterfaceNode(reference));
                    }
                }
            }
        }

        return result;
    }

ParseMembers

This is where things get really ugly. Know that I will be extracting a method to get rid of the 4x code duplication here, and some of that logic will have to go into a recursive function; like I said I went as far as I could with the naive approach, so this is as far as the naive approach got me without recursing (not without cursing!):

    private IEnumerable<ISyntaxTree> ParseMembers(string[] content, ref int currentLine)
    {
        //todo: refactor / extract methods/classes, and recurse

        var result = new List<ISyntaxTree>();
        var attributeParser = new AttributeParser();

        var pattern = @"((?<keyword>Public|Private|Friend)\s)?(?<keyword>Property|Function|Sub)\s+((?<keyword>Get|Let|Set)\s+)?(?<name>[a-zA-Z][a-zA-Z0-9_]*)(\((?<parameters>.*)\))?(\s+As\s+(?<type>.*))?$";
        var regex = new Regex(pattern);

        while (currentLine < content.Length)
        {
            if (content[currentLine].Trim().StartsWith("'"))
            {
                // comment node
                currentLine++;
                continue;
            }

            var match = regex.Match(content[currentLine]);
            if (match.Success)
            {
                var modifier = match.Groups["keyword"].Captures[0].Value;
                if (!new[]{ ReservedKeywords.Sub, ReservedKeywords.Function, ReservedKeywords.Property }.Contains(modifier))
                {
                    if (match.Groups["keyword"].Captures[1].Value == ReservedKeywords.Property)
                    {
                        var keyword = match.Groups["keyword"].Captures[2].Value;
                        var node = new PropertyNode(modifier, match.Groups["name"].Value,
                                                    keyword, match.Groups["parameters"].Value,
                                                    match.Groups["type"].Value);
                        var body = new CodeBlockNode(match.Groups["name"].Value);
                        currentLine++;
                        while (content[currentLine].Trim() != string.Format("{0} {1}", ReservedKeywords.End, match.Groups["keyword"].Captures[1].Value))
                        {
                            var attribute = attributeParser.Parse(content[currentLine]);
                            if (attribute != null)
                            {
                                node.AddAttribute(attribute);
                            }
                            else
                            {
                                var trimmed = content[currentLine].Trim();
                                if (Regex.IsMatch(trimmed, @"If\s.*Then$"))
                                {
                                    var ifBlock = new CodeBlockNode(content[currentLine]);
                                    currentLine++;
                                    while (content[currentLine].Trim() != string.Format("{0} {1}", ReservedKeywords.End, ReservedKeywords.If))
                                    {
                                        ifBlock.Nodes.Add(new CodeBlockNode(content[currentLine]));
                                        currentLine++;
                                    }
                                    currentLine++;
                                    body.Nodes.Add(ifBlock);
                                }
                                else if (Regex.IsMatch(trimmed, @"For\s.*$"))
                                {
                                    var forLoopBlock = new CodeBlockNode(content[currentLine]);
                                    currentLine++;
                                    while (content[currentLine].Trim() != ReservedKeywords.Next)
                                    {
                                        forLoopBlock.Nodes.Add(new CodeBlockNode(content[currentLine]));
                                        currentLine++;
                                    }
                                    currentLine++;
                                    body.Nodes.Add(forLoopBlock);
                                }

                                body.Nodes.Add(new CodeBlockNode(content[currentLine]));
                            }
                            currentLine++;
                        }

                        node.Nodes.Add(body);
                        result.Add(node);
                    }
                    else
                    {
                        var keyword = match.Groups["keyword"].Captures[1].Value;
                        var node = new MethodNode(modifier, match.Groups["name"].Value,
                                                    keyword, match.Groups["parameters"].Value,
                                                    match.Groups["type"].Value);
                        var body = new CodeBlockNode(match.Groups["name"].Value);
                        currentLine++;
                        while (content[currentLine].Trim() != string.Format("{0} {1}", ReservedKeywords.End, keyword))
                        {
                            var attribute = attributeParser.Parse(content[currentLine]);
                            if (attribute != null)
                            {
                                node.AddAttribute(attribute);
                            }
                            else
                            {
                                var trimmed = content[currentLine].Trim();
                                if (Regex.IsMatch(trimmed, @"If\s.*Then$"))
                                {
                                    var ifBlock = new CodeBlockNode(content[currentLine]);
                                    currentLine++;
                                    while (content[currentLine].Trim() != string.Format("{0} {1}", ReservedKeywords.End, ReservedKeywords.If))
                                    {
                                        ifBlock.Nodes.Add(new CodeBlockNode(content[currentLine]));
                                        currentLine++;
                                    }
                                    currentLine++;
                                    body.Nodes.Add(ifBlock);
                                }
                                else if (Regex.IsMatch(trimmed, @"For\s.*$"))
                                {
                                    var forLoopBlock = new CodeBlockNode(content[currentLine]);
                                    currentLine++;
                                    while (content[currentLine].Trim() != ReservedKeywords.Next)
                                    {
                                        forLoopBlock.Nodes.Add(new CodeBlockNode(content[currentLine]));
                                        currentLine++;
                                    }
                                    currentLine++;
                                    body.Nodes.Add(forLoopBlock);
                                }

                                body.Nodes.Add(new CodeBlockNode(content[currentLine]));
                            }
                            currentLine++;
                        }

                        node.Nodes.Add(body);
                        result.Add(node);
                    }
                }
                else
                {
                    if (match.Groups["keyword"].Captures[0].Value == ReservedKeywords.Property)
                    {
                        var keyword = match.Groups["keyword"].Captures[1].Value;
                        var node = new PropertyNode(null, match.Groups["name"].Value,
                                                    keyword, match.Groups["parameters"].Value,
                                                    match.Groups["type"].Value);
                        var body = new CodeBlockNode(match.Groups["name"].Value);
                        currentLine++;
                        while (content[currentLine].Trim() != string.Format("{0} {1}", ReservedKeywords.End, keyword))
                        {
                            var attribute = attributeParser.Parse(content[currentLine]);
                            if (attribute != null)
                            {
                                node.AddAttribute(attribute);
                            }
                            else
                            {
                                var trimmed = content[currentLine].Trim();
                                if (Regex.IsMatch(trimmed, @"If\s.*Then$"))
                                {
                                    var ifBlock = new CodeBlockNode(content[currentLine]);
                                    currentLine++;
                                    while (content[currentLine].Trim() != string.Format("{0} {1}", ReservedKeywords.End, ReservedKeywords.If))
                                    {
                                        ifBlock.Nodes.Add(new CodeBlockNode(content[currentLine]));
                                        currentLine++;
                                    }
                                    currentLine++;
                                    body.Nodes.Add(ifBlock);
                                }
                                else if (Regex.IsMatch(trimmed, @"For\s.*$"))
                                {
                                    var forLoopBlock = new CodeBlockNode(content[currentLine]);
                                    currentLine++;
                                    while (content[currentLine].Trim() != ReservedKeywords.Next)
                                    {
                                        forLoopBlock.Nodes.Add(new CodeBlockNode(content[currentLine]));
                                        currentLine++;
                                    }
                                    currentLine++;
                                    body.Nodes.Add(forLoopBlock);
                                }

                                body.Nodes.Add(new CodeBlockNode(content[currentLine]));
                            }

                            currentLine++;
                        }

                        node.Nodes.Add(body);
                        result.Add(node);
                    }
                    else
                    {
                        var keyword = match.Groups["keyword"].Captures[1].Value;
                        var node = new MethodNode(null, match.Groups["name"].Value,
                                                    keyword, match.Groups["parameters"].Value,
                                                    match.Groups["type"].Value);
                        var body = new CodeBlockNode(match.Groups["name"].Value);
                        currentLine++;
                        while (content[currentLine].Trim() != string.Format("{0} {1}", ReservedKeywords.End, keyword))
                        {
                            var attribute = attributeParser.Parse(content[currentLine]);
                            if (attribute != null)
                            {
                                node.AddAttribute(attribute);
                            }
                            else
                            {
                                var trimmed = content[currentLine].Trim();
                                if (Regex.IsMatch(trimmed, @"If\s.*Then$"))
                                {
                                    var ifBlock = new CodeBlockNode(content[currentLine]);
                                    currentLine++;
                                    while (content[currentLine].Trim() != string.Format("{0} {1}", ReservedKeywords.End, ReservedKeywords.If))
                                    {
                                        ifBlock.Nodes.Add(new CodeBlockNode(content[currentLine]));
                                        currentLine++;
                                    }

                                    currentLine++;
                                    body.Nodes.Add(ifBlock);
                                }
                                else if (Regex.IsMatch(trimmed, @"For\s.*$"))
                                {
                                    var forLoopBlock = new CodeBlockNode(content[currentLine]);
                                    currentLine++;
                                    while (content[currentLine].Trim() != ReservedKeywords.Next)
                                    {
                                        forLoopBlock.Nodes.Add(new CodeBlockNode(content[currentLine]));
                                        currentLine++;
                                    }

                                    currentLine++;
                                    body.Nodes.Add(forLoopBlock);
                                }

                                body.Nodes.Add(new CodeBlockNode(content[currentLine]));
                            }

                            currentLine++;
                        }

                        node.Nodes.Add(body);
                        result.Add(node);
                    }
                }
            }

            currentLine++;
        }

        return result;
    }
}

I feel at one point or another something like a Builder Pattern might emerge from the design as I refactor.

---

Here's the AttributeParser class, which is really just an abstract factory:

public class AttributeParser : IAttributeParser
{
    public IAttribute Parse(string instruction)
    {
        var syntax = @"^Attribute\s(?<Member>[a-zA-Z]+\.)?(?<Name>VB_\w+)\s=\s(?<Value>.*)$";
        var regex = new Regex(syntax);

        if (!regex.IsMatch(instruction))
        {
            return null;
        }

        var match = regex.Match(instruction);
        var member = match.Groups["Member"].Value;
        var name = match.Groups["Name"].Value;
        var value = match.Groups["Value"].Value;

        if (!string.IsNullOrEmpty(member))
        {
            return new MemberAttribute(name, value, member);
        }
        else
        {
            return new Attribute(name, value);
        }
    }   
}

I think I'm going to need more of those. Is Parser a good name for it?

Answer 1

In ParseFileHeader you repeat yourself a lot. Trim after every content[...]. You can Trim using loop

    for (int i = 0; i < content.Length; i++)
        content[i] = content[i].Trim();

This code...

        attributes.Add(attributeParser.Parse(content[8].Trim()));
        attributes.Add(attributeParser.Parse(content[9].Trim()));
        attributes.Add(attributeParser.Parse(content[10].Trim()));
        attributes.Add(attributeParser.Parse(content[11].Trim()));
        attributes.Add(attributeParser.Parse(content[12].Trim()));

... can be writen as

        foreach (var index in new[] { 8,9,10,11,12)
            attributes.Add(attributeParser.Parse(content[index])

ParseMembers is too massive. Split it into several methods. You have

private IEnumerable<ISyntaxTree> ParseMembers(string[] content, ref int currentLine)
{
    ....

    if (match.Success)
    {
        // Wall of code
    }
    else
    {
        // Wall of code
    }
}

Create KeywordMatched, KeywordNotMatched and thanks to it you splitted your wall of code to two smallers methods. It's way easier to read. Then go to KeywordMatched

if (!new[] { ReservedKeywords.Sub, ReservedKeywords.Function, ReservedKeywords.Property }.Contains(modifier))
{
    // Wall of code
}
else
{
    // Wall of code
}

And repeat process. Create ParseSubOrFunctionOrProperty and ParseOther or whateva meaningful name. After while you will have code where there are many small function with easy to read names.

EDIT: One more thing

var pattern = @"((?<keyword>Public|Private|Friend)...";
var regex = new Regex(pattern);

Use better names. What is pattern? correctKeyword or what?

Answer 2

Most of the time I don't want to read code. For example there is a bug related to data source behavior

var multiUse            = // I DON'T CARE!
var persistable         = // I DON'T CARE!
var dataBindingBehavior = // I DON'T CARE!
var dataSourceBehavior  = // I DON'T CARE!
var mtsTransactionMode  = content[6].Trim();

I can afford to not read how multiUse is defined. It's not what I'm looking for. I know it because you use meaningful names.

var match = regex.Match(content[currentLine]);
if (match.Success)

In this code I know that you try to match current line to regex but what regex? I track regex definitions and now I have to read and understand verry long and complicated regex @"((?<keyword>Public|Private|Friend)\s)?(?<keyword>Property|Function|Sub)\s+((?<keyword>Get|Let|Set)\s+)?(?<name>[a-zA-Z][a-zA-Z0-9_]*)(\((?<parameters>.*)\))?(\s+As\s+(?<type>.*))?$";.

No thanks! You can change pattern to patternAnyKeyword and regex to regexAnyKeyword. Then I have

var match = regexAnyKeyword.Match(content[currentLine]);
if (match.Success)

I understand it easily! Match is anyKeyword. But let's go further and change match to matchAnyKeyword and then I have

var matchAnyKeyword = regexAnyKeyword.Match(content[currentLine]);
if (matchAnyKeyword.Success)

When I'm reading it's

var matchAnyKeyword = // I DON'T CARE!
if (matchAnyKeyword.Success)

For example you have short and commented if

// comment node
if (content[currentLine].Trim().StartsWith("'"))
{
    currentLine++;
    continue;
}

but few lines later I see if (!new[]{ ReservedKeywords.Sub, ReservedKeywords.Function, ReservedKeywords.Property }.Contains(modifier)) you should comment what it does for example

bool isSubOrFunctionOrProperty = new[]{ ReservedKeywords.Sub, ReservedKeywords.Function, ReservedKeywords.Property }.Contains(modifier));
if (! isSubOrFunctionOrProperty )

After while you have code like

bool isSingleLineComment = // I DON'T CARE
if (isSingleLineComment)
{
    // 3 lines of code
}
else if (isFunction)
{
    // 5 lines of code
}
else if (isFor)
{
    // 30 lines of code
}

30 lines of code is a lot, time to create new method. It's very easy! Extract it and name ParseFor. You have now function with one resposibilty. Easy to test, easy to understand.

I notice that you have

var attribute = attributeParser.Parse(content[currentLine]);
if (attribute != null)
// 50 lines of code

repeated in 3 places. Your method has 250 lines of code. Extract method and ParseMembers will reduce to 100 lines + you get new method with 50 lines of code.

Answer 3

Couple of things I would do to the AttributeParser abstract factory:

1. Bring the creation of the Regex out of the Parse method since it is the same each call.

2. The if..else each with a return in it is a bit wordy. I like the ternary operator just for these instances.

3. Same advice in (1) goes for the Regexes in the ParseFileHeader, ParseDeclarations and ParseMembers methods of the CodeFileParser class.

4. In the ParseDeclarations method, I'd reverse the if (isDeclarationSection) { ... } into if (!isDeclarationSection) { continue; } as I'm a proponent of "fail fast".

5. ParseMembers is one ginormous method. I can't quite tell exactly everything it does. Or, more to the point, it does seem to do everything. I see a lot of if..elses around that look like good break-up areas into other methods - perhaps ParseProperty, ParseLoop, ParseConditional, ParseMethod, etc.

6. I'm going to noodle on the other parts of the code and update this answer in a bit.

The refinagled code:

public class AttributeParser : IAttributeParser
{
    private const string Syntax = @"^Attribute\s(?<Member>[a-zA-Z]+\.)?(?<Name>VB_\w+)\s=\s(?<Value>.*)$";

    private static readonly Regex regex = new Regex(Syntax, RegexOptions.Compiled);

    public IAttribute Parse(string instruction)
    {
        if (!regex.IsMatch(instruction))
        {
            return null;
        }

        var match = regex.Match(instruction);
        var member = match.Groups["Member"].Value;
        var name = match.Groups["Name"].Value;
        var value = match.Groups["Value"].Value;

        return string.IsNullOrEmpty(member)
            ? new Attribute(name, value)
            : new MemberAttribute(name, value, member);
    }
}