I’m working on converting a library to use dependency injection. I’ve come across this (simplified) example of code that needs to be refactored:
public class MessageQueueManager {
public static readonly MessageQueueManager Instance =
new MessageQueueManager();
private readonly MessageQueue _highPriority;
private readonly MessageQueue _lowPriority;
private MessageQueueManager() {
_highPriority = new MessageQueue(1);
_lowPriority = new MessageQueue(2);
}
}
public class MessageQueue {
private int _priority;
public MessageQueue(int priority) => _priority = priority;
public void QueueMessage(Message msg) {
_queue.Add(msg);
MessageQueueManager.Instance.NotifyMessageQueued(this);
}
}
public class Message {
public Message(string text, Action onDismiss) { ... }
private void Dismiss() {
_onDismiss();
MessageQueueManager.Instance.MessageDismissed(this);
}
}
//to display a message:
MyQueue.QueueMessage(new Message(...));
My first attempt is this shining gem:
public class MessageQueueManager : IMessageQueueManager {
private readonly IMessageQueue _highPriority;
private readonly IMessageQueue _lowPriority;
private MessageQueueManager(
Func<IMessageQueueManager, int,
/* several other dependencies*/
IMessageQueue> queueConstructor,
/*several other dependencies*/)
{
_highPriority = queueConstructor(this, 1/*, several other deps*/);
_lowPriority = queueConstructor(this, 2/*, several other deps*/);
}
}
public class MessageQueue : IMessageQueue {
private readonly IMessageQueueManager _owner;
private readonly int _priority;
public MessageQueue(
IMessageQueueManager owner, int priority,
/*several other dependencies*/)
{
_owner = owner;
_priority = priority;
/*deal with several other dependencies*/
}
public void QueueMessage(IMessage msg)
{
_msg.Manager = _owner;
_queue.Add(msg);
_owner.NotifyMessageQueued(this);
}
}
public class Message : IMessage {
public IMessageQueueManager Manager {get; set;}
public Message(string text, Action onDismiss)
{
//...
}
private void Dismiss() {
_onDismiss();
Manager.MessageDismissed(this);
}
}
MyQueue.QueueMessage(new Message(...));
So... I don’t like it.
Note also that other places have constructed their own MessageQueue
s which are not directly owned by the manager but still interact with the same system. The two provided by MessageQueueManager
are just the defaults most places will use.
Is there a cleaner way of dealing with this? Does it make more sense to have the Message’s constructor take in the MessageQueueManager? Does every place that constructs a new Message then need to be injected with that manager? (It’s a big library with messages all over, and I’m trying to do it a few pieces at a time, so that would be a big task, although it must be done eventually...)
I am in the early stages. I plan to eventually use some DI library, although I haven’t settled on which one, and I don’t know how it would really help in this instance for creating new Messages specifically. If it were some all-encompassing object that I could pass around to create messages and fetch managers for me that would be helpful, but apparently that’s not “proper dependency injection” and more of a “service locator“ which is apparently a bad thing, I guess?