patterncsharpMinor

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

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

principlesreadingmifareprogrammingwritingcardfrom

Problem

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 =

Solution

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:

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

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

-
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 for the mapping but that's just my opinion.

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

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

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

-
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 _CommandFormats
= new Dictionary
{
{ 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(IC

Code Snippets

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

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

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

    p

Context

StackExchange Code Review Q#62905, answer score: 3

Revisions (0)

No revisions yet.