HiveBrain v1.2.0
Get Started
← Back to all entries
principlecsharpMinor

Is there a better design pattern for a SDK?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
designbettersdkfortherepattern

Problem

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 Devi

Solution

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

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



-
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"))
{
    // ...
}


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

Code Snippets

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();
        }
    }

    //...
}
using (var device = new Device("COM1"))
{
    // ...
}
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;
    }
}

Context

StackExchange Code Review Q#70442, answer score: 3

Revisions (0)

No revisions yet.