1

Consider the construction of the FixAcceptor type below. The code snippet is part of a unit test.

var logSource = LogSource.OnMessage | LogSource.Event;
var stateStore = new StateStore();

var newOrderSingleMessageValidator = new NewOrderSingleMessageMessageValidator();
var newOrderSingleMessageHandler = new NewOrderSingleMessageHandler(this.logger, newOrderSingleMessageValidator, stateStore);

// More specific message handlers to come ...

var messageHandler = new AcceptorMessageHandler(this.logger, logSource, newOrderSingleMessageHandler);

var messageStoreFactory = new MemoryStoreFactory();
var sessionSettings = new SessionSettings("test_acceptor.cfg");
var logFactory = new FileLogFactory(sessionSettings);

var acceptor = new FixAcceptor(messageHandler, messageStoreFactory, sessionSettings, logFactory);

The creation of FixAcceptor feels wrong and confusing to me. Especially as there will be more specific message handlers in the future.

As I have to new-up many dependencies here, I was looking for a creational design pattern, that I can apply to the AcceptorMessageHandler or FixAcceptor type. I thought the builder pattern could be a match.

But after reading the checklist e.g. provided here: Builder Pattern I am not so sure anymore. Especially the following condition is not fulfilled

  1. Decide if a common input and many possible representations is the problem at hand

There will always be only one possible representation. Sure I could hard-wire all my dependencies and make the creation of my FixAcceptor type more easy. But then, how about testability?

Further things I don't like about the code:

  1. Passing around the cross-cutting concern/aspect this.logger.
  2. The NewOrderSingleMessageHandler is responsible for message validation and processing. Is this a violation of the SRP?

So the question is:

What is the cleanest way of constructing my types here? Which pattern should I apply?

Constructor of FixAcceptor

public class FixAcceptor
{
    private readonly ThreadedSocketAcceptor acceptor;

    // ... 

    public FixAcceptor
    (
        IAcceptorMessageHandler messageHandler, // external type
        IMessageStoreFactory messageStoreFactory, // external type
        SessionSettings sessionSettings, // external type
        ILogFactory logFactory // external type
    )
    {
        this.acceptor = new ThreadedSocketAcceptor
        (
            messageHandler,
            messageStoreFactory,
            sessionSettings,
            logFactory
        );
    }

    // ...
}

Constructor of NewOrderSingleMessageHandler

// The abstract MessageHandlers implements the validation
public class NewOrderSingleMessageHandler : MessageHandler<NewOrderSingle>
{
    private readonly IStateStore<NewOrderSingle> stateStore;

    public NewOrderSingleMessageHandler
    (
        ILogger logger, 
        IValidator<NewOrderSingle> validator,
        IStateStore<NewOrderSingle> stateStore
    ) : base(logger, validator)
    {
        this.stateStore = stateStore;
    }

    public override void Process(NewOrderSingle message, SessionID sessionId)
    {
        // ...
    }
}

Constructor of AcceptorMessageHandler

public class AcceptorMessageHandler : MessageCracker, IAcceptorMessageHandler
{
    private readonly ILogger logger;
    private readonly LogSource logSource;
    private readonly IMessageHandler<NewOrderSingle> newOrderSingleMessageHandler;

    public AcceptorMessageHandler
    (
        ILogger logger, 
        LogSource logSource,
        IMessageHandler<NewOrderSingle> newOrderSingleMessageHandler
    )
    {
        this.logger = logger;
        this.logSource = logSource;
        this.newOrderSingleMessageHandler = newOrderSingleMessageHandler;
    }

    // Lots of callbacks following... 
}
  • It's kinda hard to tell what you're trying to do from the code snippets alone. It'd probably be helpful if you could either **(1)** rewrite a minimal abstract analog to demonstrate the problem in conceptual terms; **_or_** **(2)** explain what the code's meant to do and a bit of the logic behind it. – Nat Mar 18 '20 at 09:00
  • 2
    Does this answer your question? [Should I use Dependency Injection or static factories?](https://softwareengineering.stackexchange.com/questions/190120/should-i-use-dependency-injection-or-static-factories) – Doc Brown Mar 18 '20 at 09:00
  • @DocBrown Thx for replying. A facade would help and I have tried to combine e.g. the `IAcceptorMessageHandler`, `IMessageStoreFactory`, `SessionSettings`, `ILogFactory` into a new type called `FixSettings`. But then the problem just moves one abstraction away, but still remains. – Matthias Güntert Mar 18 '20 at 09:07
  • 1
    OP, are you purposefully avoiding using an [IoC container](https://stackoverflow.com/questions/871405/why-do-i-need-an-ioc-container-as-opposed-to-straightforward-di-code) to solve this problem? I'm not seeing any reason why it would not be a good fit here. – John Wu Mar 18 '20 at 18:29
  • @JohnWu You are absolutely right. I *solved* it by using an IoC container now. I guess I should restate my question to something like "how can I make testing easier if I have complex objects?". Currently I am experimenting with `Autofac.Extras.Moq` which seems very promising. – Matthias Güntert Mar 19 '20 at 07:14

1 Answers1

3

You mentioned this as your main issue with the Builder pattern:

Especially the following condition is not fulfilled

Decide if a common input and many possible representations is the problem at hand

That is simply because this description refers to the "GoF builder pattern", which solves a different problem. What you are looking for is probably Joshua Bloch's Builder pattern, which solves the "telescoping constructor problem".

Passing around the cross-cutting concern/aspect this.logger.

The alternatives would be to either use a Singleton or a Service Locator, both are often seen as anti-patterns. In this specific case, the usage of a service locator may be fine, but you have to check its pros and cons carefully. It is a trade-off, you have to find out by yourself what works best.

The NewOrderSingleMessageHandler is responsible for message validation and processing. Is this a violation of the SRP?

Is it really responsible for both? In your code, it seems there is an instance of NewOrderSingleMessageMessageValidator injected into NewOrderSingleMessageHandler, so I guess the latter delegates validation tasks to the former, which does not look like a real violation of the SRP to me.

Of course, without knowing what your code really does, I can only make a guess here, but it might be possible split this up into 3 classes:

  • NewOrderSingleMessageMessageValidator for validation,

  • NewOrderSingleMessageHandler for unvalidated message handling, and

  • NewOrderSingleMessageValidatingHandler, which takes an instance of each of the former classes and orchestrates their validation and message handling jobs.

That would follow the SRP "by the book". But be be careful, it is easy to overdesign such a solution, and sometimes applying the SRP exhaustively is simply not worth the effort. The SRP is not an end in itself, it is a means to an end, so decide for yourself if that separation is really necessary to create useful unit tests.

Doc Brown
  • 199,015
  • 33
  • 367
  • 565