views:

221

answers:

3

Pretty new to dependency injection and I'm trying to figure out if this is an anti pattern.

Let's say I have 3 assemblies:

Foo.Shared - this has all the interfaces
Foo.Users - references Foo.Shared
Foo.Payment - references Foo.Shared

Foo.Users needs an object that is built within Foo.Payment, and Foo.Payment also needs stuff from Foo.Users. This creates some sort of circular dependency.

I have defined an interface in Foo.Shared that proxies the Dependency Injection framework I'm using (in this case NInject).

public interface IDependencyResolver
{
    T Get<T>();
}

In the container application, I have an implementation of this interface:

public class DependencyResolver:IDependencyResolver
{
    private readonly IKernel _kernel;

    public DependencyResolver(IKernel kernel)
    {
        _kernel = kernel;
    }

    public T Get<T>()
    {
        return _kernel.Get<T>();
    }
}

The configuration looks like this:

public class MyModule:StandardModule
{
    public override void Load()
    {
        Bind<IDependencyResolver>().To<DependencyResolver>().WithArgument("kernel", Kernel);
        Bind<Foo.Shared.ISomeType>().To<Foo.Payment.SomeType>(); // <- binding to different assembly
        ...
    }
}

This allows me to instantiate a new object of Foo.Payment.SomeType from inside Foo.Users without needing a direct reference:

public class UserAccounts:IUserAccounts
{
    private ISomeType _someType;
    public UserAccounts(IDependencyResolver dependencyResolver)
    {
        _someType = dependencyResolver.Get<ISomeType>(); // <- this essentially creates a new instance of Foo.Payment.SomeType
    }
}

This makes it unclear what the exact dependencies of the UserAccounts class are in this instance, which makes me think it's not a good practice.

How else can I accomplish this?

Any thoughts?

+1  A: 

That does seem a little odd to me. Is it possible to separate the logic which needs both references out into a third assembly to break the dependencies and avoid the risk?

ForeverDebugging
+1  A: 

I agree with ForeverDebugging - it would be good to eliminate the circular dependency. See if you can separate the classes like this:

  • Foo.Payment.dll: Classes that deal only with payment, not with users
  • Foo.Users.dll: Classes that deal only with users, not with payment
  • Foo.UserPayment.dll: Classes that deal with both payment and users

Then you have one assembly that references two others, but no circle of dependencies.

If you do have a circular dependency between assemblies, it doesn't necessarily mean you have a circular dependency between classes. For example, suppose you have these dependencies:

  • Foo.Users.UserAccounts depends on Foo.Shared.IPaymentHistory, which is implemented by Foo.Payment.PaymentHistory.
  • A different payment class, Foo.Payment.PaymentGateway, depends on Foo.Shared.IUserAccounts. IUserAccounts is implemented by Foo.Users.UserAccounts.

Assume there are no other dependencies.

Here there is a circle of assemblies that will depend on each other at runtime in your application (though they don't depend on each other at compile time, since they go through the shared DLL). But there is no circle of classes that depend on each other, at compile time or at runtime.

In this case, you should still be able to use your IoC container normally, without adding an extra level of indirection. In your MyModule, just bind each interface to the appropriate concrete type. Make each class accept its dependencies as arguments to the constructor. When your top-level application code needs an instance of a class, let it ask the IoC container for the class. Let the IoC container worry about finding everything that class depends on.



If you do end up with a circular dependency between classes, you probably need to use property injection (aka setter injection) on one of the classes, instead of constructor injection. I don't use Ninject, but it does support property injection - here is the documentation.

Normally IoC containers use constructor injection - they pass dependencies in to the constructor of the class that depends on them. But this doesn't work when there is a circular dependency. If classes A and B depend on each other, you'd need to pass an instance of class A in to the constructor of class B. But in order to create an A, you need to pass an instance of class B into its constructor. It's a chicken-and-egg problem.

With property injection, you tell your IoC container to first call the constructor, and then set a property on the constructed object. Normally this is used for optional dependencies, such as loggers. But you could also use it to break a circular dependency between two classes that require each other.

This isn't pretty though, and I'd definitely recommend refactoring your classes to eliminate circular dependencies.

Richard Beier
+2  A: 

Althought somewhat controversial: yes, this is an anti-pattern. It's known as a Service Locator and while some consider it a proper design pattern, I consider it an anti-pattern.

This issue is that usage of e.g. your UserAccounts class becomes implicit instead of explicit. While the constructor states that it needs an IDependencyResolver, it doesn't state what should go in it. If you pass it an IDependencyResolver that can't resolve ISomeType, it's going to throw.

What's worse is that at later iterations, you may be tempted to resolve some other type from within UserAccounts. It's going to compile just fine, but is likely to throw at run-time if/when the type can't be resolved.

Don't go that route.

From the information given, it's impossible to tell you exactly how you should solve your particular problem with circular dependencies, but I'd suggest that you rethink your design. In many cases, circular references are a symptom of Leaky Abstractions, so perhaps if you remodel your API a bit, it will go away - it's often surprising how small changes are required.

In general, the solution to any problem is adding another layer of indirection. If you truly need to have objects from both libraries collaborating tightly, you can typically introduce an intermediate broker.

  • In many cases, a Publish/subscribe model works well.
  • The Mediator pattern may provide an alternative if communication must go both ways.
  • You can also introduce an Abstract Factory to retrieve the instance you need as you need it, instead of requiring it to be wired up immediately.
Mark Seemann
That's what I thought the dependencies are not clear, so it must be an antipattern. :) An abstract factory was my first choice, but it overcomplicated the code. In the real application I need to create lots of separate types, not just one. I either hardcode each different factory method, or use generics to associate an interface to a concrete class (also hardcoded). But I lose the power of the dependency injection framework this way, and it will become really messy to configure the bindings, even to the point of needing manual dependency injection/type resolver code using reflection.
legenden
There's always a price to be paid :) I don't agree that it becomes messy to configure the container - it may become quite long-winded and verbose, but it's going to consist of a lot of almost declarative code, which is an excellent tradeoff. Much of the long-windedness you may be able to resolve with convention-based configuration - particularly if you end up with a lot of similar Abstract Factories that must all be configured in the same way. Castle Windsor has functionality that enables you to configure by convention in a few simple statements - I don't know if NInject can do this as well...
Mark Seemann