10
\$\begingroup\$

I am new with SignalR and still a newbie with IoC Container SimpleInjector and I am wondering if there are any side effects and/or if I'm save with my chosen approach.

In my web app I have a SetupHub that invokes a method on an object resolved with SimpleInjector. This method may do anything that I'm not aware about. Maybe it contains Tasks and it may consume x units of time.

During this Setup process I want to inform the client of the progress. This is my current implementation and it works fine:

public class SetupHub : Hub
{
    public void Start()
    {
        // My SimpleInjector resolve wrapper
        IocContainer.Instance.GetInstance<ISetup>().Start(UpdateProgress);
    }

    private void UpdateProgress(double percentage)
    {
        Clients.Caller.updateProgress(percentage);
    }
}


public class Setup : ISetup
{
    public void Start(Action<double> updateProgress)
    {
        Task.Factory.StartNew(() =>
                              {
                                  updateProgress(10); // Bogus progress
                                  Thread.Sleep(100);  // Simulate x units of time
                                  CreateRoles();
                                  updateProgress(25);
                                  Thread.Sleep(400);
                                  CreateUsers();
                                  updateProgress(60);
                                  Thread.Sleep(200);
                                  CreateRelations();
                                  updateProgress(100);
                                  //CreateRelationGroups();
                              });
    }
}

public interface ISetup
{
    void Start(Action<double> updateProgress);
}
\$\endgroup\$

2 Answers 2

6
\$\begingroup\$

Service Locator Alert!!

Your SetupHub class has a hardwired dependency on the IoC container itself - this is not dependency injection. When DI is done right, you cannot tell whether you're using an IoC container or if you're injecting the dependencies by hand - because the only place in your entire program that needs the IoC container, is the composition root.

This is an anti-pattern:

public void Start()
{
    // My SimpleInjector resolve wrapper
    IocContainer.Instance.GetInstance<ISetup>().Start(UpdateProgress);
}

It also seems IocContainer.Instance is a Singleton instance. Service Locator and Singleton are two patterns that directly contradict Inversion of Control and Dependency Injection.

Instead, you should be injecting the ISetup dependency through the constructor, along with the method you're depending on, like this:

public class SetupHub : Hub
{
    private readonly ISetup _setup;
    private readonly Action<double> _updateProgress;

    public SetupHub(ISetup setup, Action<double> updateProgress)
    {
        _setup = setup;
        _updateProgress = updateProgress;
    }

    public void Start()
    {
        _setup.Start(_updateProgress);
    }
}

Wiring this up with the IoC container is going to be tricky, but it could be simplified if the ISetup was injected in a factory class' constructor - first we need an abstraction for the Hub:

public interface IHub
{
    void Start();
}

Now let's create a factory class whose sole responsibility is to create IHub instances:

public class SetupHubFactory
{
    private readonly ISetup _setup;

    public SetupHubFactory(ISetup setup)
    {
        _setup = setup;
    }

    public IHub Create(Action<double> updateProgress)
    {
        return new SetupHub(_setup, updateProgress);
    }
}

Make that factory an abstraction:

public class SetupHubFactory : IHubFactory
{
    // ...
} 

public interface IHubFactory
{
    IHub Create(action<double> updateProgress);
}

This IHubFactory is an abstract factory - the client code receives any HubFactory, and works against an abstraction, instead of being tied to a single specific implementation:

public class ClientObject
{
    private readonly IHubFactory _hubFactory;

    public ClientObject(IHubFactory hubFactory)
    {
        _hubFactory = hubFactory;
    }

    public void DoSomething()
    {
        // hub is of type IHub - ClientObject isn't tied to a specific implementation.
        // inject a new implementation of the `IHubFactory` and no changes needed,
        // this method will create an instance of the new hub implementation:
        var hub = _hubFactory.Create(UpdateProgressBar);
        hub.Start();
    }

    private void UpdateProgressBar(double progress)
    {
        // ...
    }
}

DI statically declares the dependencies in your constructors. If a class has too many dependencies / constructor arguments, it's generally a sign that you're starting to break the Single Responsibility Principle.

Then you have a composition root / entry point somewhere:

public void Run()
{
    // 1. Instantiate your favorite IoC container.
    // 2. Configure your favorite IoC container.
    // 3. Resolve the entire app's dependency graph at once.
 
    // 4. Profit: // 
    var client = _ioc.GetInstance<ClientObject>();
    client.DoSomething();

    /*  */

    // or..
    // go with Poor Man's DI if you don't have a favorite IoC container:
    var setup = new Setup();
    var factory = new SetupHubFactory(setup);
    var client = new ClientObject(factory);

    client.DoSomething();
}

It's your IoC container's job to new up the dependencies and automagically inject them into all the required constructors when you ask for a ClientObject ("ask" is important here). It works automagically because all the classes involved in the dependency graph tell exactly everything they need ("tell" is important here); only infrastructure code directly works with the IoC container, the rest of the application is blissfully unaware of its existence.

Tell, don't ask.

Also, with DI/IoC you need to follow the Hollywood Principle:

Don't call them, they'll call you!

(don't call into the IoC container, the IoC container will call into your constructor)

\$\endgroup\$
6
\$\begingroup\$

Currently, your setup doesn't allow for any cancellation or error handling. The latter is an implementation detail, so I guess you've handled that in the actual code. I would consider the following:

  • Return the started task out of Start
  • Will you ever need to provide a result from the started task? If so, provide an overload for Func<double,T>
  • Provide cancellation support via a cancellation token (if business logic dictates this is acceptable)
  • Provide a Stop method that also that can stop the task created by Start
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.