2
\$\begingroup\$

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?

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Interesting Idea that you pose here about putting Format in Command. As for why i moved the logic for what you call Format is because it is about 150 lines worth of code to format the byte array properly. I used to have it in Command, but i felt it was too much so I decided to make it an extension. As a final note my implementation of Device does indeed implement IDisposable I just forgot to add it in above. Your example though seems better than my implementation so I am going to do it your way. Thank you. \$\endgroup\$ – Robert Snyder Nov 21 '14 at 11:21

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.