5
\$\begingroup\$

I have an app that takes in "commands" in the form of strings that are parsed, and then passed off to a specific class based on the first word in the string.

Every string command is comprised of two or three words separated by spaces, and can also have an '=' followed by more words/numbers separated by commas.

Some examples:

add layer layerName=path,style - commands with an = will follow this format. Three "parts" before the =, and any number of "parts" after, separated by commas.

assign database C:\temp folder\temp.mdb - commands without an = will follow this format. At most three "parts".

Right now, I have a List comprised of all the possible "first" words, which are action verbs.

I parse the string command into a string[], and switch on the first element to determine the correct function to call.

Here's the List containing the possible action verbs (shortened for better readability):

//list of all types of commands. Add to this list if more commands are created.
private static readonly List<string> Commands = new List<string>
    {"add", "assign", "cancel", "reload"}; //the actual list has ~50 strings

private static readonly string[] EmptyString = { string.Empty };

Here's my parse function:

/// <summary>
/// Given a command string, will parse it
/// </summary>
public static void StartParse(string command)
{
    if (string.IsNullOrWhiteSpace(command))  //make sure the command isn't empty
    {
        Log("Command is empty.");
        return;
    }

    string textToParse = command;

    if (textToParse.Contains('='))
    {
        string[] parts = textToParse.Split(new[] { '=' }, 2); //split the command into two parts
        string[] secondPart;

        //special case, only one command will have this format: 
        //ex: add layer layerName=filepath| ( OBJECTID = 10 ) OR ( OBJECTID = 20),styleName
        if (parts[1].Contains('|') && parts[1].Contains('(')) 
        {
            string secondParts = "";

            for (int i = 1; i < parts.Length; i++)
            {
                secondParts += parts[i];
            }

            secondPart = secondParts.Replace(@"""", "").Split(',');
        }
        else
        {
            secondPart = parts.Last().Replace(@"""", "").Split(',');
        }

        string[] firstPart = parts.First().Replace(@"""", "").Split(' '); //get rid of any quotes and split by spaces

        //if a filepath is in the first part of a command and contains spaces, it could be split into more than three parts
        //Note: if the above is true, the filepath will always start at the THIRD parameter 
        //ex: use database c:\users\test folder\temppath.mdb=...
        if (firstPart.Length > 3) 
        {
            StringBuilder sb = new StringBuilder();

            for (int i = 2; i < firstPart.Length; i++) //file path will always start at the third parameter
            {
                sb.Append(firstPart[i] + " "); //make the different parts of the filepath one string
            }

            firstPart = new[] { firstPart[0], firstPart[1], sb.ToString() };
        }

        string[] allParts = MergeArrays(firstPart, secondPart); //make one array out of the two parts of the command
        allParts[0] = allParts[0].ToLower(); //make everything lower

        //If first element is not in the command list
        if (!Commands.Contains(allParts.First()))
        {
            Log(allParts.First() + " is not a proper command.");
        }

        CallCommand(allParts);
    }
    else //if there is no '=' in the command
    {
        //creates a string array out of the command string, split by spaces
        string[] parts = TextParser.ParseText(textToParse, ' ', '"').ToArray(); 
        parts[0] = parts[0].ToLower();

        //If first element is not in the command list
        if (!Commands.Contains(parts.First()))
        {
            Log(parts.First() + " is not a proper command.");
        }

        CallCommand(parts);
    }
}

Here's the function that determines which function to call using a switch statement:

/// <summary>
/// Calls the correct command function or sends off a message based on the command verb. 
/// The command verb is always the first argument.
/// </summary>
/// <param name="command">An array of strings representing all the arguments of a commmand.</param>
public static void CallCommand(string[] command)
{
    string commandVerb = command.First().ToLower();

    //shortened for better readability, actual switch has ~50 cases
    switch (commandVerb)
    {
        case "add":
            AddCommand.AddObj(command);
            break;
        case "assign":
            AssignCommand.ParseCommand(command);
            break;
        case "cancel":
            CancelCommand.ParseCommand(command);
            break;
        case "reload":
            ReloadCommand.ParseCommand(command);
            break;
        default:
            Log("This command doesn't exist.");
            break;
    }
}

/// <summary>
/// Merges two arrays into one
/// </summary>
/// <typeparam name="T"></typeparam>
/// <param name="first"></param>
/// <param name="second"></param>
/// <returns>Returns the merged array</returns>
public static T[] MergeArrays<T>(T[] first, T[] second)
{
    T[] result = new T[first.Length + second.Length];
    Array.Copy(first, result, first.Length);
    Array.Copy(second, 0, result, first.Length, second.Length);
    return result;
}

Here's a helper parser class I'm using:

public static class TextParser
{
    /// <summary>
    /// Parses text given a delimiter and a text qualifier
    /// </summary>
    public static IEnumerable<string> ParseText(string line, char delimiter, char textQualifier)
    {

        if (line == null)
            yield break;

        bool inString = false;

        StringBuilder token = new StringBuilder();

        for (int i = 0; i < line.Length; i++)
        {
            var currentChar = line[i];

            var prevChar = '\0';
            prevChar = i > 0 ? line[i - 1] : '\0';

            var nextChar = '\0';
            nextChar = i + 1 < line.Length ? line[i + 1] : '\0';

            if (currentChar == textQualifier && (prevChar == '\0' || prevChar == delimiter) && !inString)
            {
                inString = true;
                continue;
            }

            if (currentChar == textQualifier && (nextChar == '\0' || nextChar == delimiter) && inString)
            {
                inString = false;
                continue;
            }

            if (currentChar == delimiter && !inString)
            {
                yield return token.ToString();
                token = token.Remove(0, token.Length);
                continue;
            }

            token = token.Append(currentChar);

        }

        yield return token.ToString();
    }
}

Here's an example of one of the command classes (they are all set up like this):

public static class AssignCommand
{
    /// <summary>
    /// Starts parsing the command and assigns a tag to a feature object if one is found with the given name
    /// </summary>
    public static void ParseCommand(string[] command)
    {
        if (command.Length < 4)
        {
            Log("Assign command failed: must have at least four arguments total", null)
            return;
        }

        var objectName = command[2];

        var tag = command[3];

        var feature = GlobalObjects.Map.FindFeatureByName(objectName); //Map is a third party object. GlobalObjects is a global class in the app containing the Map object. I didn't make it, it's been there forever.
        if (feature != null)
        {
            feature.Tag = tag;
        }
    }
}

The priority is improving speed as much as possible, because there will be hundreds of thousands of these commands coming in.

One thing to note is that for each action verb, there is a class that matches it to handle the command, which is what you see in the switch statements. I'm not including these because they are long, but each of these have a ParseCommand function that just parses the string array into an object. I was thinking of making a base class for them, and using some kind of generics to reduce the switch statement to a single line maybe. Not sure if this would actually increase performance though.

Another thought was using a Dictionary instead of a switch statement, with the Key being the action verb and the Value being a delegate.

\$\endgroup\$
  • 1
    \$\begingroup\$ Your examples don't match your code, is that as intended? We are also missing source code for your commands. And what is TextParser? \$\endgroup\$ – dfhwze Sep 25 '19 at 15:29
  • 1
    \$\begingroup\$ What task does this code accomplish? Please tell us, and also make that the title of the question via edit. Maybe you missed the placeholder on the title element: "State the task that your code accomplishes. Make your title distinctive.". Also from How to Ask: "State what your code does in your title, not your main concerns about it.". \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Sep 25 '19 at 15:34
  • 1
    \$\begingroup\$ Sorry about that. Updated the title, and added the source code for TextParser. @dfhwze the command classes are very long so I'd rather not post, but the main part of them is that they all have a ParseCommand(string[] command) function that takes the parsed string command and turns them into specific objects. \$\endgroup\$ – pfinferno Sep 25 '19 at 16:00
  • 1
    \$\begingroup\$ Added an example of one of the command classes at the bottom. \$\endgroup\$ – pfinferno Sep 25 '19 at 16:34
  • 1
    \$\begingroup\$ yeah, I'm fine with it. \$\endgroup\$ – ShapeOfMatter Sep 26 '19 at 11:18
5
\$\begingroup\$

It looks like what you've given us is a system that takes a string and parses it into a semantic command.

About the code you've shown, I would suggest building less stuff yourself. Your whole parsing system should probably just be a (compiled) regex with capture groups. I do suggest using a dictionary instead of a switch statement. All that will probably make this system a lot smaller and easier to read, and may make it more performant.

That said, this routing system (hopefully) isn't the performance bottleneck, and you're asking how to improve performance. You need to look at the system that gets the list of command strings in the first place, and you need to look at the system(s) that run the commands.

Probably your end goal will be a stream reader feeding an asynchronous for-each loop with shared pools of connections to outside resources. But we can't see most of that system from here, and if there are "only" 100k items to process it might be overkill.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer! It's true this isn't the performance bottleneck, but I have no control over what is sending the commands and what is running the command (which is a third party control). I just wanted to do as much as possible with what I do have control. In reality, there could be ~100 commands being sent or hundreds of thousands, depending on what the user is doing. \$\endgroup\$ – pfinferno Sep 26 '19 at 1:01
  • 1
    \$\begingroup\$ Doing as much as possibly to speed up a piece of a system that's not slowing anything down isn't a good idea. You could reduce the time spend executing this code by 100, and it might make no perceivable difference in the system's performance. Furthermore, when you focus on just speed you tend to sacrifice other stuff. It's important to keep an eye out for bone-headed decisions that will slow down the code, but your priorities should probably be readability, brevity, ease of testing, and ease of troubleshooting. \$\endgroup\$ – ShapeOfMatter Sep 26 '19 at 11:16
4
\$\begingroup\$

General Observations

  • Method name StartParse suggests there would also be its complement EndParse. But there isn't. So the name is unfortunate.
  • Method StartParse does not have a return value. This makes it a bit of a black box to consumers. I would expect is to return the command that you parsed. "I was thinking of making a base class for them" -> make a base class if commands share sufficient state, but prefer to make an interface ICommand if they share some state/operations.
  • Since method StartParse is void, your silent capturing of argument checks goes unnoticed to the consumer. Prefer throwing an ArgumentException or ArgumentNullException.
  • You have too many inline comments. If you feel these comments are required to accomodate the code, you might want to reconsider whether the code was written in a readable way to begin with. And comments as .ToLower(); // make everything lower are completely redundant.
  • Method StartParse calls CallCommand. While I think it should have returned a command, calling a method name CallCommand in a parser feels very strange. CallCommand to me means that you're executing a command, not parsing some input data to a command.
  • The method body of StartParse is verbose. You should really think about writing more compact code. The alternative solution provided in the other answer using a regex would be a good option, since the input language is rather simple.
  • I see no benefit in having a context of allowed verbs List<string> Commands if the switch case in method CallCommand does not use that list to verify against. You have hard-coded the commandVerb switches. Again on bad input, you silently ignore the bad input without the consumer knowing that the command was unrecognized.
  • Class TextParser could also use some refactoring. If you have recurring code like currentChar == textQualifier && (prevChar == '\0' this should be a signal to rewrite the code to only have this line written once.
  • AssignCommand internally uses a reference to global shared static data GlobalObjects.Map. This makes testing and reusing the class hard. Consider using IoC. Rather than a static parse method, you could provide ICommandParser interface with one or more specific parser implementations. They could then be registered to a IoC container.
\$\endgroup\$
  • 1
    \$\begingroup\$ All great points. Thank you. \$\endgroup\$ – pfinferno Sep 26 '19 at 17:08

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.