3

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 MessageQueues 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?

Steven
  • 166,672
  • 24
  • 332
  • 435
Ed Marty
  • 39,590
  • 19
  • 103
  • 156
  • Use a factory. Most DI libraries provide them, their job is to wire up dependencies for instances you need to create at runtime. You inject the factory, the factory creates the object, providing the dependencies (you can pass arguments too without having to provide all the deps). The idea is not to surface the container in any object and instead have an interface that can create objects. This guards you against the implementation of the specific object you want to instantiate so you can freely add dependencies to that object without breaking your whole codebase – Charleh Mar 30 '20 at 13:04
  • Also, you don't want to manage instantiating objects yourself or that breaks the whole idea of IoC. Factories ensure this. I'd start using a DI container now rather than later, you can always swap it out if you don't like it, the whole point of containers is that they are very unintrusive, it should feel like you are just writing code without thinking about a specific container, just that DI is happening... – Charleh Mar 30 '20 at 13:05
  • https://stackoverflow.com/questions/29890223/static-class-to-dependency-injection/29892572#29892572 check this old answer of mine for DI details. – Charleh Mar 30 '20 at 13:09
  • So does that mean I can’t really update this library piecemeal? Everywhere that creates messages needs to have the factory injected all at once? – Ed Marty Mar 30 '20 at 13:17
  • @EdMarty I have several questions. **1.** What is the purpose of the `MessageQueueManager`? Why do `MessageQueue` and `Message` have to notify `MessageQueueManager` about different events? **2.** Is `Message` dismissed outside the `Message` (other object calls method `Message.Dismiss`) or from the `Message` itself? **3.** In the provided code all dependencies have event-based nature. Do you consider of using events to decouple object dependencies? Example: https://dotnetfiddle.net/Ko5c78. – Iliar Turdushev Mar 31 '20 at 01:28
  • @IliarTurdushev 1. MessageQueueManager keeps track of the currently active queue. There can only be one message at a time; if the low priority queue has a message in it, and a message is added to the high priority queue, the low priority queue is temporarily suspended until the high priority queue has finished. 2. Dismiss is called by the GUI (eg. user dismisses via button click) or programmatically if it is no longer needed. 3. There is currently some event-style design occurring, but it is for the MQM notifying external systems about the currently active message/message queue changing. – Ed Marty Mar 31 '20 at 12:51

1 Answers1

6

There are some tweaks you will have to make:

1. Do you need IMessage?

If you have multiple implementations of queue messages, then sure, otherwise, if the message contains only data, maybe this interface can be omitted.

2. Decouple MessageQueueManager from Message

If Message uses MessageQueueManager, then with dependency injection, you have to start passing MessageQueueManager to messages whenever one is being created. Assuming you might have a lot of places where messages are created without knowing the queue manager, passing MessageQueueManager as parameter might be a massive change. And you might want to avoid doing that.

Let's assume when a message can be dismissed, it is supposed to be already in one or multiple queues. In this case, we could delegate the dismiss action to the queues where the message got added to, and it makes sense that the message queues know their manager.

So you need something like this:

public class Message : IMessage
{
    public Message(string text, Action onDismiss) { }

    // This is set by MessageQueue when the message is being added to the queue
    internal IMessageQueue Queue { get; set; }

    public void Dismiss()
    {
        //_onDismiss();

        if (Queue == null)
        {
            throw new InvalidOperationException("Message not queued.");
        }

        Queue.DismissMessage(this);
    }
}

3. MessageQueue creation

MessageQueue creation needs to be centralized. This is so that once a queue is instantiated, you can set MessageQueueManager to it so the queue can process NotifyMessageQueued and MessageDismissed.

Like what you have already tried (injecting queueConstructor), you need to, as per others said,

Use a factory.

This factory here is supposed to be your own factory as per the Dependency inversion principle. You should refrain from using any DI framework specific factory like a ILifetimeScope in Autofac. This is because dependency injection is a tool and is supposed to be optional. You need dependency inversion more than dependency injection. So I'd recommend you not tie your types to one DI framework.

public interface IMessageQueueFactory
{ 
    // Those are shotcuts to CreatePriority(N)
    IMessageQueue CreateLowPriority();
    IMessageQueue CreateHighPriority();
    IMessageQueue CreatePriority(int priority);
}

// Forgive the verbosal name here. It indicates the factory creates instance of MessageQueue.
public class MessageQueueMessageQueueFactory : IMessageQueueFactory
{
    private Lazy<IMessageQueueManager> _manager;
    
    // The reason for using Lazy<T> here is becuase we have circular dependency in this design:
    // MessageQueueManager -> MessageQueueMessageQueueFactory -> MessageQueueManager
    // No all DI framework can handle this, so we have to defer resolving manager instance 
    // Also because queue manager is a singleton instance in the container, so deferred resolving
    // doesn't cause much pain such as scoping issues.
    public MessageQueueMessageQueueFactory(Lazy<IMessageQueueManager> manager)
    {
        _manager = manager;
    }

    public IMessageQueue CreateHighPriority()
    {
        return CreatePriority(1);
    }

    public IMessageQueue CreateLowPriority()
    {
        return CreatePriority(2);
    }

    public IMessageQueue CreatePriority(int priority)
    {
        return new MessageQueue(priority, _manager);
    }
}

It is fine if you need to inject framework specific factory into MessageQueueMessageQueueFactory, because it is supposed to be an adapter here converting your own type creation to DI type resolution.

And MessageQueue now looks like:

public class MessageQueue : IMessageQueue
{
    private int _priority;
    private Lazy<IMessageQueueManager> _manager;

    // Use internal constructor to encourage using factory to instantiate message queue.
    internal MessageQueue(int priority, Lazy<IMessageQueueManager> manager)
    {
        _priority = priority;
        _manager = manager;
    }

    public void QueueMessage(IMessage msg)
    {
        _queue.Add(msg);
        ((Message)msg).Queue = this;
        _manager.Value.NotifyMessageQueued(this);
    }
    
    public void DismissMessage(IMessage msg)
    {
        // So we have a type cast here. Depends on your message queue implemenation,
        // this is ok or not.
        // For example, if this is a RabbitMQ message queue, of course the message 
        // instance must also be a RabbitMQ message, so cast is fine.
        // However, if the message queue is a base class for other message queues, this 
        // should be avoided.
        // And this also goes back to the point: Do you really need an IMessage interface?
        if (((Message)msg).Queue != this)
        {
            throw new InvalidOperationException("Message is not queued by current instance.");
        }
        
        _manager.Value.MessageDismissed(this);
    }
}

4. Message queue creation in MessageQueueManager

Now we have the factory for instantiating message queues, we could use it in constructor of MessageQueueManager:

public class MessageQueueManager : IMessageQueueManager
{
    private readonly IMessageQueue _highPriority;
    private readonly IMessageQueue _lowPriority;

    public MessageQueueManager(IMessageQueueFactory queueFactory)
    {
        _highPriority = queueFactory.CreateHighPriority();
        _lowPriority = queueFactory.CreateLowPriority();
    }
}

Note that because we want to use dependency injection, and MessageQueueManager now uses IMessageQueueFactory, so we need to change it from a real singleton to DI singleton, by removing the Instance property and registering the type as singleton

5. Do you need to handle queue family?

Say if you have multiple queue implementations, you probably don't want to mix up queue specific types. For example, you wouldn't want to add a RabbitMQ message to a MSMQ queue. If this is the case, you could use Abstract factory pattern:

public interface IMessageQueueProvider
{ 
    IMessageQueueManager CreateManager();
    IMessageQueueFactory CreateFactory();
    IMessage CreateMessage();
}

But, this is a bit out of scope.

Finally

Register the new types. I use Microsoft Extensions Dependency Injection as an example:

var services = new ServiceCollection();
services.AddSingleton<IMessageQueueManager, MessageQueueManager>();
services.AddSingleton<IMessageQueueFactory, MessageQueueMessageQueueFactory>();

// Most DI containers now should support Lazy<T>, in case if it is not, you could add
// explicit registration.
services.AddTransient(
  provider => new Lazy<IMessageQueueManager>(
    () => provider.GetRequiredService<IMessageQueueManager>()));

// This provides a way to resolve low priority queue as IMessageQueue when injected into 
// other types.
services.AddTransient(
  provider => provider.GetRequiredService<IMessageQueueFactory>().CreateLowPriority());

Here are some examples:

class Examples
{
    public Examples(
        // This is the queue manager
        IMessageQueueManager manager,
        // This is the queue factory for creating queues
        IMessageQueueFactory factory,
        // This is default queue implemenation, in this case a low priority queue
        IMessageQueue queue)
    {
        // This is queue with arbitrary priority.
        var differentPriorityQueue = factory.CreatePriority(3);

        // Queue a message and dismiss.
        var msg = new Message("Text", () => { });
        queue.QueueMessage(msg);
        msg.Dismiss();
    }
}
Community
  • 1
  • 1
weichch
  • 9,306
  • 1
  • 13
  • 25