How can I solve my constructor injection code architecture?

The current code solves the issue I had when trying to use property injection.

Problem: Every module must use constructor injection because of a circular reference that occurs when not using constructor injection with my factories and trying to use property injection

Reason: A module requires a client and each client must have its own list of instantiated modules.

What I want: I want every module that derives from BaseModule to be able to not declare a constructor and allow BaseModule to have IIrcClient injected via a property.

But I'm not sure how to use Ninject to correctly do this without getting a circular reference. Is there a way so that for each client, a new list of modules are injected into that client (where all modules then have an injected reference of the client they belong to).

.ToFactory() is an extension method of Ninject which creates a proxy based on an interface that creates an object with a dependency.

`public class BaseClient : IClient
{
private readonly IEnumerable modules;

protected BaseClient(IModuleFactory moduleFactory)
{
// each client has its own instances of modules set
// each module needs a reference to the client
this.modules = moduleFactory.Create(this);
}
}

public class BaseModule : IModule
{
private readonly IIrcClient ircClient;

// Factory will inject and create
protected BaseModule(IIrcClient ircClient)
{
this.ircClient = ircClient;
}
}

public class Bot
{
// Injected factory
public Bot(Context context, IrcClientFactory factory)
{
this.factory = factory;
this.context = context;
}

public void Start()
{
List networks = context.Networks.ToList();

foreach (var network in networks)
{
// create new client...but also
// new client gets injected with module factory
// module factory then populates modules based on client
IIrcClient

Your question isn't crystal-clear about what it is exactly that your code is supposed to be doing. We have to infer the functionality from this block of code:

public void Start()
{
    List networks = context.Networks.ToList();

    foreach (var network in networks)
    {
        // create new client...but also
        // new client gets injected with module factory
        // module factory then populates modules based on client
        IIrcClient ircClient = this.ircClientFactory.Create();
        ircClient.Connect(network);
        this.clients.Add(ircClient);
    }
}

This code involves [what appears to be] private fields that you haven't included. I'll assume the following:

private readonly Context context;
private readonly IrcClientFactory factory;

If I read this correctly, your Bot class is depending on concrete types; they should be abstractions, like IContext and IIrcClientFactory (the latter being an abstract factory).

Naming - Private Fields


This might be of personal preference, but I like my private fields prefixed with an underscore. Having _context and _factory removes the ambiguity in the constructor and then the this qualifier becomes redundant:

public Bot(Context context, IrcClientFactory factory)
{
    _factory = factory
    _context = context
}

The comment // injected factory is a useless one; prefer XML comments for that purpose:

/// 
 /// Creates a new instance of a Bot.
 /// 
 /// The data context.
 /// A factory that creates IrcClient instances.
 public Bot(Context context, IrcClientFactory factory)
 {
     _factory = factory
     _context = context
 }

As a bonus, you'll get instant IntelliSense support for your constructor and its parameters.

Back to the foreach loop; the code you have here doesn't seem to be using the fields you've initialized in your constructor:

IIrcClient ircClient = this.ircClientFactory.Create();

I would have expected:

IIrcClient ircClient = this.factory.Create();

I would rewrite it like this:

var client = _factory.Create();

Implicit Typing


The abstract factory shouldn't be revealing the implementation of its product to its client; the factory's Create() method should be returning an implementation for IClient, and shouldn't have to even know what possible implementations the factory can spit out. Your code is expecting an IIrcClient which is an abstraction, but from the rest of your code I infer that IIrcClient extends the IClient interface. Bottom line, the code doesn't care what type is returned, and it should be working against all implementations of IClient (assuming Connect(Network) is a member of IClient).


Implicit typing not only makes your code shorter and easier to read, it also emphasize that you don't really care what the actual type is.

After creating a client, the loop's body passes the network to the client through its Connect method, and then the client gets added to yet another unmentioned field, assuming:

private readonly _clients = new List();`

public void Start()
{
    var networks = context.Networks.ToList();
    foreach (var network in networks)
    {
        var client = _factory.Create();
        client.Connect(network);
        _clients.Add(client);
    }
}

If the network parameter is passed into a field of the client, and used anywhere other than in the Connect method, it would probably be best to constructor-inject the network into the client inside the factory method. If it's passed into a field of the client but isn't used anywhere other than the Connect method, it would be best to constructor-inject the network into the client inside the factory method; what I'm trying to get to, is that constructor injection should always be prioritized over other forms of injections, whenever possible. In this case, I'd probably call the IClient implementation something like ConnectedClient and inject the network through the Create method, which returns an IClient with a connection, as the ConnectedClient implementing class' name says.

Reviewing the rest of this code is very hard, I'm not seeing all the pieces of the puzzle that I'd like to see in order to be confident in anything else I'd have to say.

StackExchange Code Review Q#36578, answer score: 10