Nested switches vs domain specific parser

Question

I'm working on an application which has a defined and immutable (for our purposes) communication protocol. One of the features is that users on the controlling terminal can enter text commands that in many ways mimic interacting with a commandline application.

The structure of these commands (wrapped within a normal communication message as defined by this system) is:

<command> -<subcommand> [param] [param] [param]..

For example:

sys -messagedelay 123

Given that the commands act on the behavior of the communication manager itself (the internals of which I don't want to expose outside of that class) which of the following is the better approach?

Using a nested switch:

private SendReceiveResult HandleCommand(string command)
{
    string[] splitCommand = command.Split(CommandSplitChars);
    switch (splitCommand[0])
    {
        case "sys":
            if (splitCommand.Length < 2)
            {
                HandleError("Invalid sys command");
                break;
            }
            switch (splitCommand[1])
            {
                case "-messagedelay":
                    if (splitCommand.Length < 2 ||
                        !float.TryParse(splitCommand[1], out messagedelay))
                    {
                        HandleError("Missing or invalid messagedelay parameter");
                    }
                break;
                // .. more cases
            }
            break;
            // .. more cases
    }
}

Using a private parser class, with delegates per command:

// Private nested command parser class
private class CommandParser : IEqualityComparer<string[]>
{
    private Dictionary<string[], Func<string[], SendReceiveResult>> commandHandlers;
    private Func<string[], SendReceiveResult> defaultHandler;

    public CommandParser(Func<string[], SendReceiveResult> defaultHandler)
    {
        commandHandlers = new Dictionary<string[], Func<string[], SendReceiveResult>>(this)
        this.defaultHandler = defaultHandler;
    }

    public void AddCommandHandler(Func<string[], SendReceiveResult> commandHandler, params string[] pattern)
    {
        commandHandlers[pattern] = commandHandler;
    }

    SendReceiveResult ParseCommand(string command)
    {
        if (string.IsNullOrEmpty(command)) return SendReceiveResult.Success;

        string lowerCommand = command.ToLower();
        string[] splitCommand = lowerCommand.Split(CommandSplitChars);

        Func<string[], SendReceiveResult> commandHandler;
        if (commandHandlers.TryGetValue(splitCommand, out commandHandler))
        {
            return commandHandler(splitCommand);
        }

        return defaultHandler();
    }

    public bool Equals(string[] x, string[] y)
    {
        return y.Length > 2 &&
            y[0] == x[0] &&
            y[1] == x[1];
    }

    public int GetHashCode(string[] obj)
    {
        return obj.GetHashCode();
    }
}


// Set up of parser in enclosing class
private void SetupCommandParser()
{
    parser = new CommandParser(
        (command) =>
        {
            Log(unsupportedMessage);
            return SendReceiveResult.Success;
        });
    parser.AddCommandHandler(
    (command) => 
    {
        if (command.Length < 2 ||
            !float.TryParse(command[1], out messagedelay))
        {
            HandleError("Missing or invalid messagedelay parameter");
        } 
        return SendReceiveResult.Success;
    },
    "sys", "-messagedelay");
    // Add more commands, but in real implementation use named methods for readability
}

// Use of parser
private SendReceiveResult HandleCommand(string command)
{
    return parser.ParseCommand(command);
}

The nested switches just generally have a nasty smell about them, and while the single case is readable the real implementation that would have tens of cases would be far less so. Extending it feels like it could potentially introduce bugs easily.

The parser feels like a more complete and extensible option-and might be something I can pull out later and generalize even more to reuse. At the same time though, it feels like overkill for the task at hand.

Assuming the parser does turn out to be the better option, it could probably use regexes instead of the string array patterns, I just don't know the Regex lib well enough to slap together a quick example like this without some lookup ;)

Answer 1

Nested Switch

Nasty smell, but not such a bad idea - if it gets it done, then it could easily be refactored into a much better-looking and less error-prone form. There are already tons of posts here and on StackOverflow about refactoring switch blocks, the common way would be to use a Dictionary and map each key to a method.

Parser Class / Delegates per Command

Another, different smell, but still smelly: over-complicated. [K]eep [I]t [S]imple, [S]tupid. This is definitely shredding KISS into nano-pieces, and I have yet to understand why you would want a parser class to implement IEqualityComparer<string[]> (merely just glanced at the code).

I think the solution is simplicity: I'd start with the switch blocks, get the logic together, and refactor until satisfaction is achieved (I'd maybe end up extracting classes for each command, extracting interfaces for everything they all have in common, etc).

Or, use a command-line parser library!

Answer 2

The latter approach is, of course, object-oriented, and leads to better decoupling + easier maintenance in the long term. As always, if you have to parse two or three messages, then just write anything that works and refactor later, but why are you even asking the question then?

Here are some comments that might be useful, if you choose to consider them:

1. I don't think that a CommandParser needs to be a comparer of string arrays. You should have a separate class for doing that, and then you can reuse it properly if you ever need so.

2. The interface which it should implement is missing: if you want to split this functionality into multiple classes, then you need a contract (i.e. a IParser interface of some kind).

There are also some other approaches to consider, if you want to decouple it even further. What if your parser class would parse input data and provide intermediate results (i.e. messages, as in messaging systems) as actual objects, not caring who will use them afterwards? Once you parse the input into an instance of a "message" (whatever that is), then you essentially have a typed object which carries the actual, parsed information for someone else to process.

The thing which you need to decide in that case is whether you want to:

1. Have a parser which creates strongly-typed intermediate results (messages), and have a separate parser class for each message, or

2. Have a parser which creates weakly-typed intermediate results, but the parser is configured declaratively (even through XML files, similar to most logging frameworks, if you will) and all processing is done through a single class.

Strongly-typed hand made parsers

For the first approach, you would like your "sys -messageDelay 123" input to be parsed into an instance of a MessageDelay DTO, defined something like:

// namespace MyApp.Messages.System
class MessageDelay
{
    public float DelayInMs { get; set; }
}

Your parser interface would look something like:

public class Result<T>
{
    // Parser result (success, error) 
    public ResultType Type { get; set; }

    // Actual parsed object (if Type is Success)
    public T Value { get; set; }

    // Parsing-related message (nice to have it in case of errors)
    public string Message { get; set; }
}

public interface IParser<T>
{
    Result<T> Parse(string[] tokens);
}

Note: always create an interface for your class. This will help you define the contract and serve as a sanity check:

1. Your class really solves a certain problem for its callers
2. Doesn't try to solve too much; your basic SOLID principles

The idea is also to reuse common functionality in an abstract base class defined something like this:

public abstract class BaseParser<T> : IParser<T>
{
    public abstract string Command { get; }
    public abstract string Subcommand { get; }
    public abstract int NumberOfParameters { get; }
    protected abstract Result<T> ParseParams(string[] input);

    public Result<T> Parse(string[] input)
    {
        // first do some common checks
        if (input == null || input.Any(s => s == null))
            throw new ArgumentNullException("input");

        if (input.Length < NumberOfParameters)
            return new Result<T>() { Type = ResultType.NotEnoughData };

        if (input[0] != Command || input[1] != "-" + Subcommand)
            return new Result<T>() { Type = ResultType.NotSupported };

        // at this point we know that command and subcommand are ok
        // and we have enough tokens to parse
        try
        {
            return ParseParams(input);
        }
        catch (Exception ex)
        {
            return new Result<T>() { Type = ResultType.Error, Message = ex.ToString() };
        }
    }
}

Catching all exceptions might not be the best idea, but I leave the choice to you. Since it's only responsible for parsing, you might want to ensure that it doesn't crash your app in case of a poorly formatted input.

Your concrete implementations now need to do very little, but you still need to hand-craft each of them. By inheriting from the BaseParser, you skip the boring crap (command checking and parameter length) and get right to the main part:

public class MessageDelayParser : BaseParser<MessageDelay>
{
    public override string Command
    { get { return "sys"; } }

    public override string Subcommand
    { get { return "messagedelay"; } }

    public override int NumberOfParameters
    { get { return 3; } }

    protected override Result<MessageDelay> ParseParams(string[] input)
    {
        var delay = 0f;
        if (!float.TryParse(input[2], out delay))
            return new Result<MessageDelay>() 
            {
                Type = ResultType.Error, 
                Message = "expected float value but got " + input[2] 
            };

        return new Result<MessageDelay>()
        {
            Type = ResultType.Success,
            Value = new MessageDelay() { DelayInMs = delay }
        };
    }
}

You could now wire this through a messaging system which would allow you to remove even more plumbing (e.g. log each parsed message, specify multiple consumers for a single message, etc):

var dispatcher = new CoolMessageDispatcher();

dispatcher.AddParser<MessageDelay>(new MessageDelayParser());
dispatcher.AddParser<SomethingElse>(new SomethingElseParser());

dispatcher.AddConsumer<MessageDelay>(delay => { });
dispatcher.AddConsumer<MessageDelay>(new SomeotherConsumer());

Weakly-typed hand made parsers

Since your "protocol" is pretty simple and uniform, it can be actually described entirely using metadata:

A "Message Delay" parser starts wity sys and -messagedelay, and has a single parameter named "delay" of type float

Using C# fluent-style code:

// a nice fluent interface to make it prettier, although not necessary
var delayParser = CoolParserBuilder
    .ForCommand("sys", "messagedelay")
    .WithFriendlyName("Message Delay")
    .AddParameter<float>("delay")
    .CreateParser();

Using XML based configuration:

<Parser>
   <Name>message delay</Name>
   <Command>sys</Command>
   <SubCommand>messagedelay</SubCommand>
   <Parameters>
       <Parameter Name="delay" Type="System.Single" />
   </Parameters>
</Parser>

This concept is even more cool, because once you create your parameter parsers, you don't need to create any additional code for parsing, which means less bugs, easier testing, and better metadata in runtime.

Your parser is now completely capable of splitting the string into tokens, identifying commands and validating parameters:

- New message received: "sys -messagedelay 123"
- Split into tokens:
    Command: sys
    SubCommand: messagedelay
    Parameters: string[] { "123" }
- Found matching parser "Message Delay"
- Successfully parsed parameter "delay" of type "float": 123
- Profit!

This is a joy to maintain and extend, and provides rich logging information in runtime, but can lead to runtime errors for consumers, due to its weak-typeness. I leave the actual implementation to you.

Answer 3

as an ANTLR fan, and assuming you dont have a raging hatred for auto-generated code, I'd be remiss if parser generators didn't get mentioned here.

With a parser generator, you'd specify your desired syntax in a grammar file and the interpreter in a listener (or 'walker' or 'visitor') C# file.

//ANTLR comlang.g4 grammar file:
grammer comlang;

cmd 
    : sys
    | run 
    | status

start
    : 'sys' (msgDelay | specialTreatment)? generalArg+

run 
    : 'run' generalArg*

msgDelay
    : '-messagedelay' [0-9]+
specialTreatment
    : '-specialTreatment' ('UI_ONLY' | 'BIG_DEAL' | 'SILENT')

generalArg
    : [0-9a-zA-Z_]+

//... and so on.

And your code generating file:

//quick disclaimer: I've never worked with the C# ANTLR target, 
//only the java one, but I've used that one extensively, so I'm estimating:
class ComlangInterpreter : ComlangBaseListener{

    Command command;

    public override void exitCmd(ComlangParser.CmdContext ctx) {
        command.run();
    }

    public override void exitSys(ComlangParser.SysContext ctx) {
        SysCommand command = new SysCommand{
            messageDelay = ctx.msgDelay?.Children?.[1]?.Text ?? "",
            specialTreatment = Enum.parse(SpecialTreatment, ctx.specialTreatment?.Children?.[1].Text ?? "DEFAULT")
            args = ctx.generalArgs.select(tnode => tnode.Text).toList()
        }
        this.command = command;
    }

    public override void exitRun(ComlangParser.RunContext ctx) {
        //...
    }

    //...
}

You will also have to write some 10-20 lines of boilerplate to get ANTLR to lex and parse the text, then run your listener over it.

The two big wins in this approach are that you get to specify your language in EBNF (or an ASCII version of it), and you don't have to write the branching logic yourself. This makes the listener and grammar vastly easier to read and reason about (and have interns/newbies modify) than hand-written parsers.

Another cool win is that ANTLR does a reasonable job at best-effort recovery. You can configure it to act differently, but by default it will coalesce the tree to a close legal approximation, and give you warnings (rather annoyingly pushed to std-err by default) when it does.

then the nasty bit:

you'd have to modify your build system to tolerate the generated code that comes out of your parser generator. The tools for doing this are reasonable on the java side (ant, gradle, and maven tasks all ready to go), but I don't know how they are in .net land.

Further, for your situation, this is a pretty heavy-handed measure. If you can get away with writing your parser by hand and never touching it again, doing that is the best option. If you find yourself going back to that parser time and time again I would urge you to consider a solution involving a parser generator.