Injection doesn't work using overload constructors and this()

[thgbrb]

Hi All,

I'm face some issues when overloading constructor.

In the example below I don't receive the instance from simpleInjector, it always return null.

public class ClienteObterCommand : Command
{
    private static IClienteAppServiceRead _clienteAppServiceRead;
    public IList<ClienteDTO> clientes;

    public ClienteObterCommand(IClienteAppServiceRead clienteAppServiceRead)
    {
            _clienteAppServiceRead = clienteAppServiceRead;
    }

    public ClienteObterCommand(out IList<ClienteDTO> clientesDto) : this(_clienteAppServiceRead)
    {
        clientesDto = ClienteDTO.Domain2View(_clienteAppServiceRead.ObterTodos()).ToList();
    }
}

When I explicit request an instance to container it works.

public class ClienteObterCommand : Command
{
    private static IClienteAppServiceRead _clienteAppServiceRead;
    public IList<ClienteDTO> clientes;

    public ClienteObterCommand(IClienteAppServiceRead clienteAppServiceRead)
    {
            // BusCommand comes from inherited class
            _clienteAppServiceRead = BusCommand.GetInstance<IClienteAppServiceRead>();
    }

    public ClienteObterCommand(out IList<ClienteDTO> clientesDto) : this(_clienteAppServiceRead)
    {
        clientesDto = ClienteDTO.Domain2View(_clienteAppServiceRead.ObterTodos()).ToList();
    }
}

Thank you in advance for support and always a big congrats for this amazing work!!! :D

[dotnetjunkie]

I detected several bad practices in your code:

• ClienteObterCommand has multiple public constructors, while application components should only have one; multiple constructors is an anti-pattern. That's why OOTB Simple Injector will not even allow you to resolve this type; it tries to force to define only a single constructor on your application components.
• You inject a dependency into a constructor that is stored in a static field. This is problematic, because every new instance that is created will replace the old static reference, which can cause all sorts of problems, such as multi-threaded issues, and at least makes your application very hard to follow. In general, prevent mutable state inside your application components.
• Your constructor has an out parameter. I must admit that I didn't even know that this was possible. In general you should prevent out parameters on public parts of your API and especially on constructors. This makes your code really hard to follow and means that your constructor does more than it is supposed to do; see the following point.
• Your constructor does all sorts of initialization code, like calling the Domain2View and using the supplied dependency. Constructors should not do anything else than storing their incoming dependencies. Any other logic should only be executed after the object graph is fully constructed. This means you should move this logic out of the constructor and keep your injection constructors simple.
• Your ClienteObterCommand has a base class with dependencies of its own. You should prevent using base classes, especially when they contain or use dependencies.
• The BusCommand is a form of Service Locator, which is an anti-pattern. The use of its GetInstance<T> method hides dependencies, while all direct dependencies should be injected through the constructor. The Service Locator pattern hides those dependencies, which causes maintainability issues, makes your code harder to test, and makes your DI container completely blind and unable to warn you about any configuration mistakes you made have made.

I have no idea what you are trying to achieve with this code, so it's not possible for me to suggest a better design, but it you follow the advice as given above, this should guide you towards a better solution that solves your issues.

Good luck.

[thgbrb]

Hi Steve,

Thank you for detailed answer. The reason to use multiple constructor is because I’m create a new instance of ClienteObterCommand in a class where I can’t the mandatory parameter (an interface).

//Guid clienteGuid;
_bus.Send(new ClienteObterCommand(clienteGuid, out Clientes));

//string nome;
_bus.Send(new ClienteObterCommand(nome, out Clientes));

When I create this instance I call an constructor that fit with passed parameters and each constructor overloaded use : this(_clienteAppServiceRead) in order to receive an instance of interface using SimpleInjector.

I don’t know if is lack of knowledge mine or wrong approach, but I cannot figure out how to solve this problem. Do you have any thoughts?

[dotnetjunkie]

The core problem here is that you are mixing data and behavior. Any class in your system should either be data or behavior, not both. Your ClienteObterCommand is a message that you sent (probably over the wire). However, that command contains logic and that's why you inject services into it. You should separate this. Just take a look at this article for instance. Here you see that commands are pure behaviorless DTOs. The behavior is moved into a service that can handle that command.

Mixing data and behavior leads to lots of problems. For instance, it becomes impossible to abstract the logic inside the command to allow unit testing or adding cross-cutting concerns without having to make sweeping changes. It's impossible to do so, because the consumer takes a hard dependency on a component (your command is a component (something with behavior) instead of just a data container). Pure data objects can just be seen as extensions of an interface (they form the contract of your application) and because they don't have any data, there is no need to hide them behind an abstraction.

Since your command seems to produce a result rather than changing the state of the system, I think this operation is actually a query. This article describes a much better way to define queries in your system.

I hope this helps.

[thgbrb]

Hi Steve,

I have worked on the solutions showed in your blog and it is just amazing, mainly the query processor. It will save a lot of time!! thank you!!! About commands, I have done some test using decorators and it is helping solve some problem, but the main issue I cant figure out how to solve.. or maybe the solution already is the best solution. All the code I have written in this threat is to solve it: do not use container.GetInstance<>(). Considering the code below, you see when I create a new instance of CommandHandlerDecorator I must pass a instance of my commandHandler! In my understand Simple Injector should create this instance to me!

How would you solve it? I have wrote below a complete test class used to this tests. Thank you again! ;)

using SimpleInjector;
using Xunit;

namespace UnitTestLibrary1
{
    public class Testing
    {
        public Container container;
        public Testing()
        {
            container = new Container();
            container.RegisterCollection(typeof(ICommandHandler<>), new[] { typeof(ICommandHandler<>) });
            container.RegisterDecorator(typeof(ICommandHandler<>), typeof(CommandHandlerDecorator<>));
            container.Register(typeof(IMessages), typeof(Messages));
        }

        [Fact]
        public void DeveCriarDecorator()
        {
            // Create a new instance of Messages with a message
            var message = new Messages() { Message = "Testing" };

            // Create new command (only data) 
            var command = new CustomerMovedCommand() { Messages = message };

            // How to solve? Get a instance of ICommandHandler<CustomerMovedCommand>()
            var commandHandler = container.GetInstance<ICommandHandler<CustomerMovedCommand>>();

            // Create new commandHandlerDecorator
            var decorator = new CommandHandlerDecorator<CustomerMovedCommand>(commandHandler);

            // Call Handler
            decorator.Handle(command);

            // Compare a string with a static variable of Messages classe 
            // static string is set by CustomerMovedcommandHandler.Handle()
            Assert.Equal("Testing", Messages.message);
        }
    }
}

namespace UnitTestLibrary1
{
    public interface IMessages { Messages New(string msg); }

    public interface ICommandHandler<in TCommand> { void Handle(TCommand command); }

    public class Messages : IMessages
    {
        public static string message;
        public string Message { get; set; }

        public Messages New(string msg)
        {
            return new Messages() { Message = msg};
        }
    }

    public class CustomerMovedCommand
    {
        public Messages Messages { get; set; }
    }

    public class CustomerMovedCommandHandler : ICommandHandler<CustomerMovedCommand>
    {
        private readonly IMessages _messages;

        public CustomerMovedCommandHandler(IMessages messages)
        {
            _messages = messages;
        }

        public void Handle(CustomerMovedCommand command)
        {
            var commandReceived = command;
            Messages.message = _messages.New("Testing!!!").Message;

        }
    }

    public class CommandHandlerDecorator<TCommand> : ICommandHandler<TCommand>
    {
        private readonly ICommandHandler<TCommand> _decoratedHandler;

        public CommandHandlerDecorator(ICommandHandler<TCommand> decoratedHandler)
        {
            _decoratedHandler = decoratedHandler;
        }

        public void Handle(TCommand command)
        {
            _decoratedHandler.Handle(command);
        }
    }
}

[dotnetjunkie]

You're almost there. Here's is the correct configuration to get everything rolling:

var container = new Container();

container.Register(typeof(ICommandHandler<>), new[] { typeof(ICommandHandler<>).Assembly });
container.RegisterDecorator(typeof(ICommandHandler<>), typeof(CommandHandlerDecorator<>));

var command = new CustomerMovedCommand() { Messages = new Messages { Message = "Testing" } };

var commandHandler = container.GetInstance<ICommandHandler<CustomerMovedCommand>>();

commandHandler.Handle(command);

Note the following:

• Since there is a one-to-one mapping between an abstraction and an implementation, you need to use Register instead of RegisterCollection. In other words, your ICommandHandler<CustomerMovedCommand> has only one implementation, namely CustomerMovedCommandHandler. RegisterCollection is used when you want to resolve IEnumerable<T> that possibly contains multiple implementations. Say for instance you have an IValidator<T> abstraction and have multiple implementations for IValidator<CustomerMovedCommand>. Since that's not what you want, you need to use Register.
• You registered an abstraction as one of the elements in RegisterCollection. Register collection is very versatile and in case one of the supplied types is an abstraction, it will do a call-back to the container to search for a registration for that abstraction. This can be very useful, but not a typical scenario. In case you use RegisterCollection, you will typically supply it with a list of concrete types, e.g. container.RegisterCollection(typeof(IValidator<>), new[] { typeof(CustomerValidator), typeof(GoldCustomerValidator), ... });.
• Instead of passing a list of types to Register, I pass in a list of assemblies. In that case Simple Injector will go look for the types for you. This is much more flexible, since it allows you to add new types, without having to make any changes to your registrations.
• When you register a decorator in Simple Injector using RegisterDecorator, Simple Injector will wrap a created type with a decorator for you. You don't have to create the decorator manually; that would defeat the whole purpose of registering this decorator. Just register the decorator and Simple Injector will do the rest.

[thgbrb]

Hi Steve,

Amazing explanation! I worked very hard this week and solve a lot of problema based on your inputs. I was able to throw alway all that constructor overload and mainly the List by reference (out) which I was using to receive values from queries. Thank you so much for all help!!! Before I finish this threat I would like to check with you if is possible automatically register all handlers e queries using simple injector.

Today, for each handlers, query or layer I do an explicit registration like:

// Registro de Camadas Titulos new BusRegister().Register(typeof(ITituloAppService), typeof(TituloAppService)); new BusRegister().Register(typeof(ITituloService), typeof(TituloService)); new BusRegister().Register(typeof(ITituloRepositoryWrite), typeof(MockTituloRepository));

//Handlers

region QueryHandlers

//#Backlog 579

region Documento

new BusRegister().Register(typeof(IQueryHandler<DocumentoObterPorGuidQuery, Documento>), typeof(DocumentoObterPorGuidHandler)); new BusRegister().Register(typeof(IQueryHandler<DocumentoObterDocumentosPorGuidQuery, IEnumerable>), typeof(DocumentoObterPorGuidHandler));

endregion

Is there a way to do it simplest? I thought that a registration like _container.Register(typeof(IQueryHandler<,>), new[] { typeof(IQueryHandler<,>).Assembly }); could work but doesnt!

Thank you so much again!

Best Regards, Thiago

[dotnetjunkie]

Your last registration is correct:

_container.Register(typeof(IQueryHandler<,>), new[] { typeof(IQueryHandler<,>).Assembly });

This will batch-register all non-generic, non-abstract implementations from typeof(IQueryHandler<,>).Assembly that implement IQueryHandler<,>. If this doesn't work, take a close look at the registrations in the container. It might be that your handlers live in a different assembly.

[thgbrb]

Hi Steve,

You are right, the IQueryHandler<> interface and QueryHandler<> class are in an assembly called Bus.InMemory while the queries are in another.

// Bus.InMemory //BusQuery class public TResult Process(IQuery query) { //logic }

// Cadastros

// Cadastro.Query.PessoasBase.Queries { public class PessoaBaseObterPessoasPorAliasQuery : IQuery<IEnumerable> { // logic }

//Cadastro.Query.PessoasBase.Handlers public class PessoaBaseObterPorGuidHandler : IQueryHandler<PessoaBaseObterPorGuidQuery, Pessoa>, IQueryHandler<PessoaBaseObterPessoasPorGuidQuery, IEnumerable> { // logic }

I just have read and testing according Simple injector documentation (advanced scenario) the Batch registration but without success. Do you know anyway to do the batch registration of this?

Thks!,

Best Regards, Thiago

Em 21 de ago de 2016, à(s) 17:53, Steven notifications@github.com escreveu:

Your last registration is correct:

_container.Register(typeof(IQueryHandler<,>), new[] { typeof(IQueryHandler<,>).Assembly }); This will batch-register all non-generic, non-abstract implementations from typeof(IQueryHandler<,>).Assembly that implement IQueryHandler<,>. If this doesn't work, take a close look at the registrations in the container. It might be that your handlers live in a different assembly.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/simpleinjector/SimpleInjector/issues/285#issuecomment-241281396, or mute the thread https://github.com/notifications/unsubscribe-auth/AD3WFZLmyu2tKmn0JvPWCnSngEmF5qbDks5qiLqzgaJpZM4JibGo.