Variable parser for the Hack programming language

I am optimizing the variable parser for my programming language, and also trying to make it more readable and concise (precedence: readable and concise > efficient). What is the best way to improve this code and make the best out of each line, and make it easier to understand for contributors?

namespace pro_compiler
{
    /* Handles all the variable declarations */
    public class VarParser : Parser
    {
        public VarParser( )
            : base()
        {    
        }

        public string Parse(ref string line)
        {
            CheckVariable(ref line);
            return line;
        }

        public void CheckVariable(ref string line)
        {
            /* Check current line for the "var" keyword */
            if (line.StartsWith("var"))
            {
                string decl = line;       
                decl += ';';
                line = decl;
                CheckFunctions(ref line);
            }    
        }

        /* check if the var declaration contains any functions
         * e.g var input = readln; */
        public string CheckFunctions(ref string line)
        {
            var words = line.Split(' '); //check wether each word is a function
            line = ""; // clear the current line

            foreach (var word in words)
            {               
                string w = word.Trim(); //trim all leading and trailing spaces.

                foreach(var pair in FuncTable.Instance.NormSet)
                {
                    if (w.StartsWith(pair.Key)) // if the word is a function 
                    {
                        w = w.Replace(pair.Key, pair.Value + "( )"); // replace it with the C# equivalent
                    }
                }

                w = ((w == words[0]) ? "" : " ") + w; // reinsert trailing and leading spaces
                line += w; // add the words back to the current line
            }

            return line;
        }
    }
}

Also, I have used a lot of the C# sta

A few observations...

General

• It would be great if you could include a syntax snippets of how a line in your programming language would look so we can test our solutions against the expected output. I had to go to your Google Code project and search for an actual syntax example in your source files

• Your parsing functionality is still very rudimentary. (I didn't change that.) It can't yet handle function arguments and it doesn't work if there are no spaces in between words. Also, if there are spaces in front of the variable declaration, the function will not get translated at all (this I fixed)

• It's usually advisable to stick with library functions and not to reinvent the wheel, especially since you have stated that readability trumps efficiency in your scenario. .NET's string functions are mostly fine

• There's no need to explicitly specify the default constructor as it (along with it's call to the argument-less constructor of Parser) will be automatically generated by the compiler. Leaving it out means one less thing for your readers and contributors to worry about

Naming issues

• According to the comment just above the class definition, VarParser handles all the variable declarations. Although this makes sense, I'd suggest you ponder on finding a more descriptive name, such as VariableDeclarationParser or at least VariableParser

• The function name CheckVariable is lacking at best, or misinformation at worst. It tells us it is "checking a variable" but in reality it isn't - it's checking if the current line contains a variable and, if it does, it will process the line - otherwise it will do nothing. An adequate name for that would be ParseLineIfItContainsAVariableDeclaration. Of course we don't use that, but rather we realize that the function needs to be split up to separate the check from the processing, so we choose two names for our new functions:

• ContainsVariableDeclaration(string line) will return a bool indicating if we should process the line

• ParseLine(string line) will return a string containing the parsed line

• FuncTable is a very vague name for your Singleton containing the (also vague) NormSet - consider something like HackToCSharpMapper (or just LanguageMapper) and FunctionMappings or something along those lines instead of NormSet

• CheckFunctions doesn't check anything, rather it seems to parse functions. Therefore, name it ParseFunctionsIn(string line)

• I'm not saying you should choose the exact names I recommended, just try to think of the name that captures what the function does best and don't imply a function is doing something it doesn't do

• Naming is important because it forces us to define what a given function, variable or class should do. When we end up with long names (see example above), we know that our code isn't well-structured and needs to be changed so that we follow the principles of OOP.

A function should do one thing. It should do it well. It should do it only.

            — Robert C. Martin in Clean Code: A Handbook of Agile Software Craftsmanship


Comments

Almost all of your comments can be made obsolete by choosing proper names and generally making the code as expressive as possible. IMHO, we should avoid comments that tell us what a particular statement does, rather we should use them only to clarify why we have made an unusal decision.

Or, to put it like Uncle Bob, I view every comment I write as a failure to express myself in code.

So I deleted most of your comments from the refactored version below because the names now stand for themselves, without the need for further clarification.

Dangerous and unusual things

• Are you sure there won't be any whitespace before the var keyword? I've added a call to String.TrimStart() to make sure we don't stumble over this edge case

• In your class, I see absolutely no reason to pass the line parameter by reference. This should be avoided because it makes your code more unpredictable - one can no longer be sure whether the function has a side effect on the input parameter

• CheckFunctions returns a string, but the return value is never used because you rely on the ref parameter - be consistent!

• Why are CheckVariable and CheckFunctions public? Too me, it looks like they should only ever be accessed from within that class

• What if someone passes in a line that doesn't contain a variable declaration? At the moment, there will be no warning at all, the result will just be the same as the input - I suggest changing your program to throw an ArgumentException if it is not passed a valid declaration

Loops

• Your nested foreach loop is unnecessary and bad for performance because it loops over all possibilities. As further investigation of the rest of your source code on Google Code shows, FuncTable.Instance.NormSet is a Dictionary and therefore suitable for fast access by key.

• Note that in your original code, you kept g

StackExchange Code Review Q#7318, answer score: 14