1

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.

6
  • Have a look at the Law of Demeter. Jul 30 at 13:56
  • @RobertHarvey Thanks for the edit. Indeed, this design breaks the law of Demeter, which is why this felt off to me. The alternative would be to have a two visitor visiting OutMessage. One being MessageNotSentVisitor and the other MessageSentVisitor. The different OutMessage would expose necessary info for the visitor to process. I still have mixed feelings about that design.
    – Cromm
    Jul 30 at 14:02
  • 1
    I'm not that worried about SRP at this point. Setting MessageSent or MessageNotSent status is still a single responsibility. Jul 30 at 14:03
  • 3
    What if you just moved your status handling into your OutMessage object? You can move the switch there, and then you may not even need the PmsSenderService at all. Jul 30 at 14:06
  • 1
    @RobertHarvey This comment would deserve to be an answer :-)
    – Christophe
    Jul 30 at 16:27
1

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<SendStatus> 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<TParam, TStatus> onExecute, Action<SendStatus> onCompletion) 
      {
        _onExecute = onExecute;
        _onCompletion = onCompletion;
      }
    
      public void Execute(TParam param) {
         var status = _onExecute(param);
         _onCompletion(status);
      }
    }


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

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

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

6
  • Thanks, I like the idea to pass a function for completion. However this does not solve my main issue "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, basically for each implementation of OutMessage, the operation will be different. Where would I store such function (that function will do DB operations).
    – Cromm
    Aug 2 at 8:09
  • 1
    @Cromm Sorry, I kind of missed that. So, if I'm not mistaken, your abstract OutMessage supertype doesn't really provide much besides a way for the factory to return different subtypes uniformly. Are you using the concrete out message type essentially as a "tag", to encode some information about the message (i.e. besides that, there's very little polymorphic behavior)? And the problem is then that you need to sort of double-dispatch on message type and the status. 1/3 Aug 2 at 23:47
  • 1
    @Cromm I think what's happening is that the way you conceptualized it all results in codependent parts of functionality being scattered in different objects and/or services. The type-based decisions are not confined to the factory. Let me think about it some more (and see what you'll reply) before I update the answer. But in the outline, there should be some way to divide things (responsibilities) up differently so that related code ends up together (high cohesion) - at least in when it comes to the high-level policies (things that orchestrate other code with narrower responsibilities). 2/3 Aug 2 at 23:48
  • 1
    @Cromm Perhaps one approach is to re-conceptualize the factory as returning "message sender" object, instead of the "parsed" message itself (the message would be embedded), but that may not work if you need the message for other things? In any case, something like that could let you separate out code by narrow cases (SRP) - for a particular message type and a particular status response (or maybe at some larger granularity) - and "assemble" them in the factory as appropriate. 3/3 Aug 2 at 23:49
  • 1
    @Cromm P.S. BTW, this re-conceptualization I mentioned is similar to Robert Harvey's comment to move the code to the OutMessage object, I'm just additionally suggesting that this entails re-thinking what this concept means (what it's used for) in your code, and to then rename it to something that better reflects the new role. When you evolve the design, you sometimes get stuck on the notions you originally came up with. Aug 2 at 23:57

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.