principlecsharpMinor
Is there a better design pattern for a SDK?
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
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:
-
Since
so now we have clear
-
I am not sure whether
In this case you can of course merge
CommandAdapterseems to be unnecessary separation of logic here. I would just move it as instance method ofCommand(CommandAdapteris 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.