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
- 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:
- Passing around the cross-cutting concern/aspect
this.logger
. - 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...
}
1 Answer 1
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, andNewOrderSingleMessageValidatingHandler
, 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.
Explore related questions
See similar questions with these tags.
IAcceptorMessageHandler
,IMessageStoreFactory
,SessionSettings
,ILogFactory
into a new type calledFixSettings
. But then the problem just moves one abstraction away, but still remains.Autofac.Extras.Moq
which seems very promising.