Better way to design this loop?

Question

I'm building a simple assembly compiler for my own custom flavor of assembly, and i have something like this as the actual code that does the compiling:

    foreach (KeyValuePair<short, string> kvp in newCommandSet)
    {
        string fullCommandString = kvp.Value;

        string instruction = fullCommandString.Split(new char[] { Convert.ToChar(" ") })[0];
        string[] parameters = fullCommandString.Split(new string[] { ", " }, StringSplitOptions.RemoveEmptyEntries);
        // this is to remove the instruction part from the first parameter. Gonna have to ensure a well formed command at some point...
        parameters[0] = parameters[0].Substring(instruction.Length + 1);
        Command currentCommand = new Command();
        switch (instruction)
        {
            case "load":
                short value = Convert.ToInt16(instruction[0]);
                byte register = Convert.ToByte(parameters[1]);
                currentCommand = CommandFactory.CreateLoadCommand(register, value);
                break;
            case "input":
                byte channel = Convert.ToByte(parameters[0]);
                register = Convert.ToByte(parameters[1]);
                currentCommand = CommandFactory.CreateInputCommand(register, channel);
                break;
            case "output":
                channel = Convert.ToByte(parameters[0]);
                register = Convert.ToByte(parameters[1]);
                currentCommand = CommandFactory.CreateInputCommand(register, channel);
                break;
            ...
        }
        ...
    }

It feels like i'm breaking about half a dozen design rules here (reusing variables and expecting well-formed input are the only ones i can spot but i bet there's more), but have no clue how to build it any better. Ideas?

Answer 1

Consider pushing the logic for interpreting the parameters into the CommandFactory. The switch statement would look as follows:

switch(instruction)
{
    case "load":
        currentCommand = CommandFactory.CreateLoadCommand(parameters);
        break;
    case "input":
        currentCommand = CommandFactory.CreateInputCommand(parameters);
        break;
    case "output":
        currentCommand = CommandFactory.CreateOutputCommand(parameters);
        break;
}

Answer 2

You might consider throwing a couple things in like a tokenizer that returns your program as a string of tokens (your splitter kind of does this). Then pass that off to a parser to create a parse tree and symbol table. Why? because without knowing your flavor of assembly, at some point you are going to want to jump to a label (subroutine) I would assume. or you will want your jump instruction to head back to the start of a loop, etc...

If you have your parse tree and symbol table set up, you will have all the addresses right there for easy insertion into your output file. It's been a long long time since I wrote a compiler, so please forgive any deviations in my little example...