Programming Principles - Reading/writing from/to a Mifare card

Question

Consider the following class (I've stripped XML doc for the sake of simplicity), which does several and slightly different operations through a serial port to read/write from/to a Mifare card. It has a specific protocol, few commands that can be called sending the specified chars to the COM Port, and have been put altogether here.

Does it break SRP? How could I change this structure for maintainability? Is there any other principle that I should be aware in this implementation (e.g. POLS)?

Update 1: I think POLS is broken when I use the DescriptionAttribute, in order to define the commands...maybe I should create my own attribute to get things properly doing what they were made for.

//{0} Block
//{1} Key
public enum MifareCommand
 {
 [Description("D{0}0000{1}")]
 StartAutoRead,
 [Description("YCC0000{1}")]
 ReadMyStructure,
 [Description("U")]
 SuspendAutoRead,
 [Description("Z")]
 ResumeAutoRead,
 [Description("B{0}0000{1}")]
 ReadBlock,
 [Description("E{0}0000{1}H{2}")]
 WriteBlock
 }
public class Mifare
{
 private const int BlockSize = 16;
 private const int ControlBlockInterval = 16;
 private const int MyStrucureSizeBlock = 8;
 private const int MyStrucureFirstBlock = 128;
 private const int AutoReadBlock = 1;
 private readonly SerialPort _serialPort;
 public string Port { get; set; }
 public string Key { get; set; }
 public Mifare()
 {
 Port = Properties.Settings.Default.MifarePort;
 Key = "FFFFFFFFFFFF";
 if (string.IsNullOrEmpty(Port))
 DetectComPort();
 _serialPort = new SerialPort(Port) { Encoding = Encoding.ASCII };
 }
 private void DetectComPort()
 {
 var query = string.Format("SELECT DeviceID FROM Win32_SerialPort WHERE Description = '{0}'", Properties.Settings.Default.MifareDevice);
 var objectSearcher = new ManagementObjectSearcher(query);
 var obj = objectSearcher.Get().Cast<ManagementObject>().FirstOrDefault();
 if (obj != null)
 Port = obj["DeviceID"].ToString();
 }
 public void SuspendAutoRead()
 {
 Send(MifareCommand.SuspendAutoRead);
 }
 public void ResumeAutoRead()
 {
 Send(MifareCommand.ResumeAutoRead);
 }
 public void StartAutoRead()
 {
 Send(MifareCommand.StartAutoRead, AutoReadBlock);
 }
 public void ReadBlock(int block)
 {
 Send(MifareCommand.ReadBlock, block);
 }
 public void ReadMyStructre()
 {
 Send(MifareCommand.ReadMyStructure);
 }
 public void WriteBlock(int block, byte[] bytes)
 {
 if (bytes.Length < BlockSize)
 Array.Resize(ref bytes, BlockSize);
 var blockString = block.ToString("X2");
 var bytesString = BitConverter.ToString(bytes).Remove("-");
 var commandString = string.Format(MifareCommand.WriteBlock.GetDescription(), blockString, Key, bytesString);
 Send(commandString);
 }
 public void WriteMyStructure(byte[] structure)
 {
 try
 {
 var lengthBytes = BitConverter.GetBytes(structure.Length);
 Array.Reverse(lengthBytes); //big-endian
 // 00 00 TT TT TT TT 00 00 00 00 00 00 00 00 00 00 
 var headerBytesToWrite = new byte[BlockSize];
 lengthBytes.CopyTo(headerBytesToWrite, 2);
 WriteBlock(MyStrucureSizeBlock, lengthBytes);
 for (var i = 0; i < structure.Length; i += BlockSize)
 {
 var bytesToWrite = structure.Skip(i)
 .Take(BlockSize)
 .ToArray();
 var block = MyStrucureFirstBlock + (i / BlockSize);
 if (block % ControlBlockInterval == 0)
 block++;
 WriteBlock(block, bytesToWrite);
 }
 }
 catch (Exception e)
 {
 Console.WriteLine(e.ToString());
 }
 }
 public void Send(MifareCommand command)
 {
 var commandString = string.Format(command.GetDescription(), Key);
 Send(commandString);
 }
 public void Send(MifareCommand command, int block)
 {
 var paramString = block.ToString("X2");
 var commandString = string.Format(command.GetDescription(), paramString, Key);
 Send(commandString);
 }
 public void Send(byte[] buffer)
 {
 try
 {
 if (!_serialPort.IsOpen)
 _serialPort.Open();
 _serialPort.Write(buffer, 0, buffer.Length);
 Thread.Sleep(5);
 var command = BitConverter.ToString(buffer);
 if (!string.IsNullOrEmpty(command))
 Console.WriteLine("Sent: '" + command.Remove(_serialPort.NewLine, "-") + "'");
 }
 catch (Exception ex)
 {
 Console.WriteLine(ex.ToString());
 }
 }
 public void Send(string command)
 {
 try
 {
 if (!_serialPort.IsOpen)
 _serialPort.Open();
 _serialPort.WriteLine(command);
 Thread.Sleep(5);
 if (!string.IsNullOrEmpty(command))
 Console.WriteLine("Sent: '" + command.Remove(_serialPort.NewLine) + "'");
 }
 catch (Exception ex)
 {
 Console.WriteLine(ex.ToString());
 }
 }
 public string Receive()
 {
 string ret;
 try
 {
 ret = _serialPort.ReadLine();
 if (!string.IsNullOrEmpty(ret))
 Console.WriteLine("Received: '" + ret + "'");
 }
 catch (TimeoutException e)
 {
 ret = "Error: " + e.Message;
 }
 _serialPort.DiscardInBuffer();
 return ret;
 }
}

Answer 1

Yes I think you do break SRP. Currently your class is pretty much untestable due to the hard-coded implicit dependency on SerialPort which is going to be very hard to mock out.

You should definitely remove the connection handling (open, close, send, receive) into a separate interface and inject that instead.

---

A few more things I noted:

1. Your two Send methods are almost identical. The string method can be refactored o use the byte[] method for example:

   public void Send(string command)
   {
    // assuming the Mifare card accepts ASCII only anyway
    Send(System.Text.Encoding.ASCII.GetBytes(command));
   }
   

2. There is an easier way to get the port names - SerialPort.GetPortNames().

3. I don't really like using the Description attribute of the enum for the format string. This seems like a misuse of the attribute and I'd prefer to use a Dictionary<MifareCommand, string> for the mapping but that's just my opinion.

4. You are catching all Exceptions in various places and write them to the console. This is usually abad idea because you don't give the caller any idea that something went and pollute stdout with the messages. It's better to use either a dedicated logger to inject or Trace.TraceError() to log the messages and rethrow the exception so the caller can deal with it.

5. You dump various messages to the console without giving the caller the option to change that behaviour. Again using an injected logger or the Trace class is a better option.

6. In your Receive method you catch the TimeoutException and format a custom return error. I think letting the caller catch the exception would be more flexible.

7. SerialPort is IDisposable because it derives from Component so whatever object owns the SerialPort should implement IDisposable as well.

So what I was thinking about is along these lines of refactoring:

public interface IConnection
{
 public void Send(string data);
 public void Send(byte[] data);
 public string Receive();
}
public class SerialConnection : IConnection, IDisposable
{
 private SerialPort _SerialPort;
 public SerialConnection() : this(null)
 {
 }
 public SerialConnection(string port)
 {
 try 
 {
 if (string.IsNullOrWhiteSpace(port))
 port = SerialPort.GetPortNames().FirstOrDefault();
 }
 catch (Win32Exception ex)
 {
 // GetPortNames() can throw this
 Trace.TraceError(ex.ToString());
 port = null;
 }
 if (string.IsNullOrWhiteSpace(port))
 throw new ArgumentException("Port name not specified and unable to determine standard port");
 _SerialPort = new SerialPort(port) { Encoding = Encoding.ASCII };
 }
 public void Send(byte[] buffer)
 {
 try
 {
 if (!_SerialPort.IsOpen)
 _SerialPort.Open();
 _SerialPort.Write(buffer, 0, buffer.Length);
 Thread.Sleep(5);
 var command = BitConverter.ToString(buffer);
 if (!string.IsNullOrEmpty(command))
 Trace.WriteLine("Sent: '" + command.Remove(_SerialPort.NewLine, "-") + "'");
 }
 catch (Exception ex)
 {
 Trace.TraceError(ex.ToString());
 throw; // re-throw and let the caller deal with it
 }
 }
 public void Send(string command)
 {
 // assuming the Mifare card accepts ASCII only anyway
 Send(System.Text.Encoding.ASCII.GetBytes(command));
 }
 public string Receive()
 {
 try
 {
 var ret = _SerialPort.ReadLine();
 if (!string.IsNullOrEmpty(ret))
 Trace.WriteLine("Received: '" + ret + "'");
 return ret;
 }
 finally
 {
 _serialPort.DiscardInBuffer();
 }
 }
 public void Dispose()
 {
 if (_SerialPort != null)
 {
 _SerialPort.Dispose();
 _SerialPort = null;
 }
 }
}

And the Mifare class would then look something like this:

public enum MifareCommand
{
 StartAutoRead,
 ReadMyStructure,
 SuspendAutoRead,
 ResumeAutoRead,
 ReadBlock,
 WriteBlock
}
public class Mifare : IDisposable
{
 private const int BlockSize = 16;
 private const int ControlBlockInterval = 16;
 private const int MyStrucureSizeBlock = 8;
 private const int MyStrucureFirstBlock = 128;
 private const int AutoReadBlock = 1;
 private readonly Dictionary<MifareCommand, string> _CommandFormats 
 = new Dictionary<MifareCommand, string> 
 {
 { StartAutoRead, "D{0}0000{1}" },
 { ReadMyStructure, "YCC0000{1}" },
 { SuspendAutoRead, "U" },
 { ResumeAutoRead, "Z" },
 { ReadBlock, "B{0}0000{1}" },
 { WriteBlock, "E{0}0000{1}H{2}" }, 
 }
 private readonly IConnection _Connection;
 public string Key { get; set; }
 public Mifare(IConnection connection)
 {
 if (_Connection == null)
 throw new ArgumentNullException("connection");
 _Connection = connection;
 Key = "FFFFFFFFFFFF";
 }
 public void SuspendAutoRead()
 {
 Send(MifareCommand.SuspendAutoRead);
 }
 public void ResumeAutoRead()
 {
 Send(MifareCommand.ResumeAutoRead);
 }
 public void StartAutoRead()
 {
 Send(MifareCommand.StartAutoRead, AutoReadBlock);
 }
 public void ReadBlock(int block)
 {
 Send(MifareCommand.ReadBlock, block);
 }
 public void ReadMyStructre()
 {
 Send(MifareCommand.ReadMyStructure);
 }
 public void WriteBlock(int block, byte[] bytes)
 {
 if (bytes.Length < BlockSize)
 Array.Resize(ref bytes, BlockSize);
 var blockString = block.ToString("X2");
 var bytesString = BitConverter.ToString(bytes).Remove("-");
 var commandString = string.Format(_CommandFormats[MifareCommand.WriteBlock], blockString, Key, bytesString);
 Send(commandString);
 }
 public void WriteMyStructure(byte[] structure)
 {
 try
 {
 var lengthBytes = BitConverter.GetBytes(structure.Length);
 Array.Reverse(lengthBytes); //big-endian
 // 00 00 TT TT TT TT 00 00 00 00 00 00 00 00 00 00 
 var headerBytesToWrite = new byte[BlockSize];
 lengthBytes.CopyTo(headerBytesToWrite, 2);
 WriteBlock(MyStrucureSizeBlock, lengthBytes);
 for (var i = 0; i < structure.Length; i += BlockSize)
 {
 var bytesToWrite = structure.Skip(i).Take(BlockSize).ToArray();
 var block = MyStrucureFirstBlock + (i / BlockSize);
 if (block % ControlBlockInterval == 0)
 block++;
 WriteBlock(block, bytesToWrite);
 }
 }
 catch (Exception e)
 {
 Trace.TraceError(e.ToString());
 throw;
 }
 }
 public void Send(MifareCommand command)
 {
 var commandString = string.Format(_CommandFormats[command], Key);
 Send(commandString);
 }
 public void Send(MifareCommand command, int block)
 {
 var paramString = block.ToString("X2");
 var commandString = string.Format(_CommandFormats[command], paramString, Key);
 Send(commandString);
 }
 public string Receive()
 {
 return _Connection.Receive();
 }
 public void Dispose()
 {
 if (_Connection != null)
 {
 _Connection.Dispose();
 _Connection = null;
 }
 }
}

• natenho: So your suggestion is to have a separated interface for open/close/send/receive. Do you mean one interface per operation? What do you think about that COM port detection method? Tks
• ChrisWue: @natenho: I've updated my answer a little bit.
• natenho: It's definitely a good and comprehensive answer! The only thing I was about to change is the log injection, it's really a weak log impl. Thank you for pointing all these things out!
• natenho: Just one more question. I'm a little confused about SRP. Keeping different methods like WriteMyStructre, WriteBlock, SuspendAutoRead, ReadBlock, etc. doesn't it smell multiple responsibilities?
• ChrisWue: @natenho: Well, in this case the functionality of the class is to deal with the Mifare protocol and to translate a request (from a programer) into the appropriate byte stream. The lines are not always clear cut and different people will have different opinions. As long as the class is of a reasonable size and your external dependencies (like connections) are properly encapsulated you are in a reasonably good shape I think.