Is there a better design pattern for a SDK?

Question

At work I decided it would be better to reverse engineer a SDK for one of our pieces of hardware. Because it is work related I can't share the exact code.. but I can give enough of a gist of the code with the following question in mind. Is there a design pattern that better reflects what I want to do?

I'm going to describe what the endgame is, then lay the code out with how to use it first, how it is implemented, and what I was thinking about for a change.

The end game is that the device I have communicates via RS232. I write a byte array to it to have it do things for me, it responds back with a similar structure. (The response is not of concern though). So what I have done is created a rather large factory class (not exactly a factory pattern, but I couldn't think of a better name). The code in small part looks similar to this

public class Command
{
    public ushort DeviceId{get;private set;}
    public byte CommandId {get; private set;}
    public byte[] Data {get; private set;}

    public Command(ushort deviceId, byte commandId, params byte[] data)
    {
        //assign to properties
    }
}
public static class CommandAdapter
{
    public static byte[] ToArray(this Command command)
    {
        //build byte array to match format needed
        return buffer;
    }
}

public class Device
{
    private ushort deviceId;
    private SerialPort devicePort;
    public Device(string comPort)
    {
        devicePort = new SerialPort(comPort, 115200);
        byte[] response = TransceiveCommand(CommandFactory.GetDeviceId());
        deviceId = BitConverter.ToUInt16(response, 0);
    }
    public byte[] FooBar()
    {
        byte mode = 1;
        byte option = 0x52;
        Command cmd = CommandFactory.DoFooBar(deviceId, mode, option)
        return TransceiveCommand(cmd);
    }

    private byte[] TransceiveCommand(Command cmd)
    {
        byte[] buffer = cmd.ToArray();
        devicePort.Write(buffer, 0, buffer.Length);
        byte[] response = new DeviceResponseParser.GetResponse(devicePort);
        return response;
    }
}

and I think that this code isn't so bad. it's the factory that I don't care much for.

public static class CommandFactory
{
    public static Command GetDeviceId()
    {
        return new Command(0, GET_DEVICE_CMD_ID);
    }

    public static Command DoFooBar(ushort deviceId, byte mode, byte option)
    {
        return new Command(deviceId, FOOBAR_CMD_ID, mode, option);
    }

    public static Command Another(ushort deviceId, byte[] data)
    {
        return new Command(deviceId, ANOTHER_CMD_ID, data);
    }

    public static Command SetSerialNumber(ushort deviceId, string newSerial)
    {
        byte[] array = new byte[newSerial.Length + 2]; //1 for 0x12, 1 for length
        array[0] = 0x12; //not sure what the 0x12 is, but it is required
        array[1] = (byte)(newSerial.Length); //+1 for null-termination
        for (int i = 0; i < newSerial.Length; i++) { array[i + 2] = (byte)newSerial[i]; }
        return new Command(deviceId, SET_SERIAL_CMD_ID, array);
    }
    //..~30 more commands

    private const byte GET_DEVICE_CMD_ID = 0x01;
    private const byte FOOBAR_CMD_ID = 0x02;
    //.. ~30 more command id's
}

In the end the commands are not going to change, so I've considered just using a byte array instead of a whole class for command. I don't like the idea of making an extension method for a byte array which makes me second guess doing that. Every command except for GetDeviceId takes in as it's first parameter the device id. So I wrote down on paper an idea about writing something like CommandFactory.CreateCommandFor(deviceId).FooBar()

or I've also thought about making a non-static Command factory pass in the deviceId and have it send back commands. Then I wouldn't have to pass in the device Id each time. Are there any design patterns that would better match what I'm trying to acheive?

Answer 1

I see this as a good design. As for now I would change only those two things and consider changing one more:

1. CommandAdapter seems to be unnecessary separation of logic here. I would just move it as instance method of Command (CommandAdapter is being removed),

2. Since Device represents low-level communication with hardware via RS232 - seems like something unmanaged going there and in fact SerialPort is IDisposable so - Device should be either:

   public class Device : IDisposable
   {
       private ushort deviceId;
       private SerialPort devicePort;
       public Device(string comPort)
       {
           devicePort = new SerialPort(comPort, 115200);
           // ...
       }
   
       public void Dispose()
       {
           Dispose(true);
           GC.SuppressFinalize(this);
       }
   
       public virtual void Dispose(bool disposing)
       {
           if (disposing)
           {
               devicePort.Dispose();
           }
       }
   
       //...
   }
   

   so now we have clear IDisposable semantics:

   using (var device = new Device("COM1"))
   {
       // ...
   }
   

3. I am not sure whether Command is used somewhere else, but currently it seems to be a class that is created just to call ToArray on it. It may be inefficent, it depends how many times it is called etc. So consider reducing Command and pass byte[] representing command directly:

   public class Command
   {
       internal static byte[] Format(ushort deviceId, byte commandId, params byte[] data)
       {
           //build byte array to match format needed
           return new byte[] { };
       }
   }
   
   public static class CommandFactory
   {
       public static byte[] GetDeviceId()
       {
           return Command.Format(0, GET_DEVICE_CMD_ID);
       }
   
       public static byte[] DoFooBar(ushort deviceId, byte mode, byte option)
       {
           return Command.Format(deviceId, FOOBAR_CMD_ID, mode, option);
       }
   
       // ...
   }
   
   public class Device : IDisposable
   {
       // ...
   
       public byte[] FooBar()
       {
           byte mode = 1;
           byte option = 0x52;
           byte[] cmd = CommandFactory.DoFooBar(deviceId, mode, option);
           return TransceiveCommand(cmd);
       }
   
       private byte[] TransceiveCommand(byte[] cmd)
       {
           devicePort.Write(cmd, 0, cmd.Length);
           byte[] response = DeviceResponseParser.GetResponse(devicePort);
           return response;
       }
   }
   

   In this case you can of course merge Command.Format into CommandFactory. This makes much less sophisticated design but if Command do not any additional value - maybe worth consideration.