1

Requirement: We need a tool that simulates a hardware device that communicates via RS232 or TCP/IP to allow us to test our main application which will communicate with the device.

Current flow:

  • User loads script
  • Parse script into commands
  • User runs script
  • Execute commands

Script / commands (simplified for discussion):

  • Connect RS232 => RS232ConnectCommand
  • Connect TCP/IP => TcpIpConnectCommand
  • Send data => SendCommand
  • Receive data => ReceiveCommand
  • Disconnect => DisconnectCommand

All commands implement the ICommand interface. The command runner simply executes a sequence of ICommand implementations sequentially thus ICommand must have an Execute exposure, pseudo code:

  • void Execute(ICommunicator context)

The Execute method takes a context argument which allows the command implementations to execute what they need to do. For instance SendCommand will call context.Send, etc.

The problem RS232ConnectCommand and TcpIpConnectCommand needs to instantiate the context to be used by subsequent commands. How do you handle this elegantly?

Solution 1: Change ICommand Execute method to:

  • ICommunicator Execute(ICommunicator context)

While it will work it seems like a code smell. All commands now need to return the context which for all commands except the connection ones will be the same context that is passed in.

Solution 2: Create an ICommunicatorWrapper (ICommunicationBroker?) which follows the decorator pattern and decorates ICommunicator. It introduces a new exposure:

  • void SetCommunicator(ICommunicator communicator)

And ICommand is changed to use the wrapper:

  • void Execute(ICommunicationWrapper context)

Seems like a cleaner solution.

Question

Is this a good design? Am I on the right track?

  • Why does solution 1 seem like a code smell to you? It looks like solution 1 is somewhat singleton based, where solution 2 is based on multiple instantiations. Also, with solution 2 you wouldn't need a context to be passed into the Execute method. – Adam Zuckerman Mar 26 '14 at 3:42
  • Commands are just objects created for each line in the script. A command factory produce the commands by parsing a line, deciding which command object to instantiate and passing the arguments for the command in the constructor. – Hawk Mar 26 '14 at 3:46
  • I am not following how what you are saying is related to my comments and question. – Adam Zuckerman Mar 26 '14 at 3:50
  • Commands implement ICommand to allow execution. When executing commands need to have something to execute against. Executing a connect command should establish a connection via RS232 or Tcp/Ip. Executing a send command would need to send data via a connection. The problem is how to deal with the connection that is created by some commands and consumed by others. There is no singleton pattern. – Hawk Mar 26 '14 at 3:53
  • 1
    Sorry, hit enter and saved previous comment and then ran into 5 min edit limit. See above comment for clearer explanation. Difference between 1 and 2 is that in 1 I pass the "connection" into all command executions. Connect commands return new connection instances. Other commands return the same connection you passed in which means extra non-sensical code when writing commands that are not connect commands. – Hawk Mar 26 '14 at 3:56
1

I think solution 2 is the better solution. You want to avoid treating connection commands as special cases. That would add complexity to your script engine and/or script language. Your commands should receive a general purpose execution context (or script state).

Some psuedo-code could be:

interface ICommand {
    void Execute(IScriptContext context);
}

interface IScriptContext {
    ICommunicator Channel { get; set; }
}

interface ICommunicator{
    int Read(byte[] buffer, int offset, int count);
    int Write(byte[] buffer, int offset, int count);
    int GetBytesAvailable();
}

class RS232CommunicationChannel : ICommunicator...
class TcpIpCommunicationChannel : ICommunicator...
class SharedMemoryCommunicationChannel : ICommunicator...

class ConnectRS232Command : ICommand {
    void Execute(IScriptContext context) {
        context.CommunicationChannel = new RS232CommunicationChannel();
    }
}

The execution loop is then very simple:

IList<ICommand> commandList = LoadCommands();
IScriptContext context = new ScriptContextImpl();
foreach(ICommand cmd in commandList) {
    cmd.Execute(context)
}
  • I like this approach because it has better separation of concern. My rationale is that the simulated internal states of the device (maintained by the scripts), and the communication channel used between the application and the device, don't seem to be suitable for combining into a single object. One could even argue that a device state could exist (e.g. it has been powered up and was initialized with configurations from ROM) before a communication channel is even established. This device state can then influence how the communication channel is established - e.g. disabling some behavior, etc. – rwong Dec 21 '14 at 22:51
0

While I am mostly ambivalent about your two design patterns above, I would tend to lean more toward solution 1. Some of my reasoning is thus:

You have a clear separation of concerns using Execute (or any command) with a parameter of the communications context. It shows that each Execute has a dependency on a context that it does not own or manage. What you do in the Execute interacts with the communications context in the same fashion (i.e., send characters, receive characters, pause transmission, etc.). This will show you what interface your communications context requires. You can reuse the object with a different communications context just by calling it again with a different object.

You can swap out your communications context for anything else that supports the interface. There's no harm if your object supports more functionality, but your object must support the minimums. It also means that if you want to add any other communications protocol (USB, UDP, eSATA, SCSI) at a later date, it should be very simple.

You can create a factory method for each of the communication contexts. This will help for contexts that can be multi-threaded as you can start a separate thread for each instance as a part of spinning up the instance. For those contexts that don't support multi-threading (e.g., Serial), you can also spin up just one instance per serial port. The factory could keep track of the "pool" of available serial ports so you don't reuse one already in use.

Lastly, this follows a very widely known and supported methodology for implementation. It is (IMO) a much more maintainable implementation.

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.