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

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...

Answer 2

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 3

Move the instruction info to a class / property bag. Create some utility methods for conversions to make your life easier. Then use a string -> delegate dictionary to map the instruction name to creation of a command. This is just a start, you can refactor this to be a great deal simpler.

Something along these lines, perhaps:

public class InstructionData
{
    public InstructionData(string fullCommandString)
    {
        string[] commandParts = fullCommandString.Split(new char[] {' ', ','}, StringSplitOptions.RemoveEmptyEntries);
        this.InstructionName = commandParts[0];
        this.parameters = commandParts.Skip(1).ToArray();
    }

    public string InstructionName { get; private set; }
    public short InstructionInt { get { return Convert.ToInt16(InstructionName[0]); } }
    private string[] parameters;
    public string GetParameter(int paramNum)
    {
        return parameters[paramNum];
    }
    public byte GetParameterAsByte(int paramNum)
    {
        return Convert.ToByte(parameters[paramNum]);
    }
}


public class SomeClass
{
    // ...
    private Dictionary<string, Func<InstructionData, Command>> commandTranslator = new Dictionary<string, Func<InstructionData, Command>>();

    private static void InitializeCommandTranslator()
    {
        commandTranslator["load"] = ins => CommandFactory.CreateLoadCommand(ins);
        commandTranslator["input"] = ins => CommandFactory.CreateInputCommand(ins);
        commandTranslator["output"] = ins => CommandFactory.CreateOutputCommand(ins);

    }

    public void SomeMethod()
    {
     // ...
        foreach (KeyValuePair<short, string> kvp in newCommandSet)
        {
         InstructionData currentInstruction = new InstructionData(kvp.Value);

            if(commandTranslator.ContainsKey(currentInstruction.InstructionName))
            {
                currentCommand = commandTranslator[currentInstruction.InstructionName](currentInstruction);
            }
        }
    }

    // ...
}