SRP, command pattern and design problem

Question

I'm refactoring a code base to have something more easier to read and follow SRP. However I'm at the point I'm unsure what the best design is.

Currently it looks like this: A thread receives a "command" to execute. The "command" is always the same, it takes the form of an encoded message to send to a WS. The message first need to be "decoded" to send the proper data to the WS. For that first step, I have a factory that will give me one of the implementation of the OutMessageParser to parse the message into one of the object to send.

    public interface IOutMessageParser
    {
        OutMessage CreateOutMessage();
    }
public abstract OutMessage
{
    abstract string GetTcpMessage(); //Used to get the message to sent to a WS
}

So, when I receive the command in the thread, I'm parsing it that way - note that the factory will pass constructor arguments depending on the message, so some OutMessageParser receive different construtor argument and they have internal properties. Then for each OutMessageParser, the CreateOutMessage method will retrieve data from DB and create an OutMessage :

var outMessage = OutMessageParserFactory.GetOutMessageParser(messageReceived).CreateOutMessage();

Then a service will send the message to the client that will try to send the message. The client returns a status (SENT, ERROR, SOCKET_EXCPETION, EXCEPTION, etc, etc)

senderService.SendMessage(outMessage)

The SendMessage looks like this :

    public class SenderService
    {
        //Constructor, internal properties, etc.
    public void SendMessage(OutMessage outMessage)
    {
        var sendStatus = _wsClient.SendDataMessage(outMessage.GetTcpMessage());
        switch(sendStatus)
        {
            //TODO Now what ????
        }
    }

}

Now, the tricky part is that, depending on the type of sendStatus (the enum) AND the concrete type of OutMessage, the DB needs to be updated, basically two main scenario exists - either the sent is successful or it's not. For any given scenario, the linked DB objects of the OutMessage needs to be updated in a certain way.

So my idea is to create an interface like this :

public interface IOutMessageCommand
{
    // these methods will update DB objects
    void ExecuteMessageSent();
void ExecuteMessageNotSent();

}

And for each OutMessage having a corresponding implementation, when creating the OutMessage, an ICommand is created as well. Then the service would look like this :

The SendMessage looks like this :

    public class SenderService
    {
        //Constructor, internal properties, etc.
    public void SendMessage(OutMessage outMessage)
    {
        var sendStatus = _wsClient.SendDataMessage(outMessage.GetTcpMessage());
        switch(sendStatus)
        {
            case SendStatus.SENT : 
                outMessage.Command().ExecuteMessageSent()
                break;
            case SendStatus.NOT_SENT: 
            case SendStatus.EXCEPTION:
                outMessage.Command().ExecuteMessageNotSent()
                break;
        }
    }

}

However this seems off... Having a simple object holding its command seems weird to me, especially that when creating the command I'll need some info from the OutMessage instance properties (passed through the Command constructor) before calling executing its methods. Is that an anti-pattern ? Because the OutMessage have the responsability to create a IOutMessageCommand instance - but it should not be its responsibility. Is there another way to do ? I've thinking about a factory to create the Command, but I'd like to avoid having a big switch over the instanceOf(OutMessage) - that would look ugly and not very practical.

Thanks in advance.

EDIT :

For now the solution I'm using is like this :

public class SenderService
{
    private readonly WsClient _wsClient;
internal SenderService(WsClient wsClient)
{
    _wsClient = wsClient;
}

public void SendOutMessage(OutMessage outMessage)
{
    outMessage.SendStatusType = _wsClient.SendDataMessage(outMessage.GetTcpMessage());
    IOutMessageVisitor outMessageVisitor;
    switch(outMessage.SendStatusType)
    {
        case SendStatusType.EXCEPTION:
        case SendStatusType.PROTOCOL_ERROR:
        case SendStatusType.NOT_SENT:
        case SendStatusType.RECV_TIMEOUT:
            outMessageVisitor=  new NotSentVisitor();
            break;
        case SendStatusType.SENT_COMPLETED:
            outMessageVisitor= new SentVisitor();
            break;
        default:
            outMessageVisitor= new NotSentVisitor();
            break;
    }
    outMessage.Accept(outMessageVisitor);
}

}

Along with the following modification, for each implementation of OutMessage I have an accept method:

        internal override void Accept(IOutMessageVisitor visitor)
        {
            visitor.Visit(this);
        }

And the IOutMessageVisitor looks like this :

    internal interface IOutMessageVisitor
    {
        void Visit(v3456Message message);
        void Visit(v23Message message);
        void Visit(v1Message message);
        // And so on for more than a dozen different message types
}

Finally both IOutMessageVisitor implementations performs, for each Visit methods, specific DB updates.

I'm not happy with this solution, but that's the best I came up with for now.

Filip Milovanović · Answer 1 · 2021-08-02T02:05:38.510

Well, don't forget that objects can store references to other executable code (other objects or functions). One thing you could do is parameterize the command by a function that does something after the operation either succeeds or fails. This can be handy if you want to have commands that execute an action, and then have certain side-effects, e.g. on a view, without them being aware of what is it that they are having side-effects on.

Presumably, the place where your factory is creating these command instances is one where you have access to code that can do something with the status:

    // NOTE: the IOutMessageCommand interface may or may not be necessary
    class OutMessageCommand : IOutMessageCommand {
      private WSClient _wsClient;
      private Action<SendStatus> _onCompletion;
  public OutMessageCommand(WSClient wsClient, Action&lt;SendStatus&gt; onCompletion) {
    _wsClient = wsClient;
    _onCompletion = onCompletion;
  }

  // You could also move this parameter to the constructor, and then have 
  // parameterless 'void SendMessage()', which may be better 
  // if you want to treat your commands polymorphically
  public void SendMessage(OutMessage outMessage) {
     var sendStatus = _wsClient.SendDataMessage(outMessage.GetTcpMessage());
     _onCompletion(sendStatus);
  }
}

And then in your factory you can do:

    var command = new OutMessageCommand(wsClient, (sendStatus) => {
      switch(sendStatus) {            
        case SendStatus.SENT : 
          // update some view, or console, or log, or something
          break;
        case SendStatus.NOT_SENT: 
        case SendStatus.EXCEPTION:
          // show/emmit an error message
          break;
      }
    });

Then just pass the command object along, and call it with:

command.SendMessage(outMessage);

If you don't want it to depend on WS client, then either create an interface for that dependency (exposing only the methods that you actually need to use), or if you only need to call SendDataMessage, then just pass in a function. You could even make the whole command generic (unless you need to be able execute commands of different types in the same way - in that case rely on interfaces and encapsulation):

    class Command<TParam, TStatus> {
      private Func<TParam, TStatus> _onExecute;
      private Action<TStatus> _onCompletion;
  public OutMessageCommand(
    Func&lt;TParam, TStatus&gt; onExecute, Action&lt;SendStatus&gt; onCompletion) 
  {
    _onExecute = onExecute;
    _onCompletion = onCompletion;
  }

  public void Execute(TParam param) {
     var status = _onExecute(param);
     _onCompletion(status);
  }
}


// --------------------------------------------------------
// Command Factory:
var sendCommand = new Command&lt;OutMessage, SendStatus&gt;(
  (outMessage) =&gt; _wsClient.SendDataMessage(outMessage.GetTcpMessage()), 
  (sendStatus) =&gt; {
    switch(sendStatus) { /* ... */ }
  });

// --------------------------------------------------------    
// Somewhere else:
_sendCommand.Execute(outMessage);

You may have to change some details to deal with asynchronous execution.

SRP, command pattern and design problem

1 Answers1