Getting rid of the ServiceLocator in the base class

Question

I have an abstract base class for command/query handlers:

public abstract class AbstractBusiness<TRequest, TResult> : IBusiness<TRequest, TResult>
    where TResult : BaseResponse
{
    public TResult Send(TRequest request)
    {
        TResult response = Activator.CreateInstance<TResult>();
        response.Success = true;

        try
        {
            IConnectionProvider connectionProvider =
                ServiceLocator.Current.GetInstance<IConnectionProvider>();

            using (DatabaseScope scope = DatabaseScopeManager.Instance.GetScope(
                connectionProvider, 
                DatabaseScopeType.UseExistingOrNewTX,
                IsolationLevel.ReadCommitted))
            {
                response = this.Handle(request);
                scope.Complete();
            }
        }
        catch (Exception exc)
        {
            response.Success = false;
            response.Message = exc.Message;
        }

        return response;
    }

    protected abstract TResult Handle(TRequest request);
}

This is how it is used:

public sealed class AccountCreateBusiness
    : AbstractBusiness<AccountCreateRequest, AccountCreateResponse>
{
    private readonly IMessageProvider messageProvider;

    public AccountCreateBusiness(IMessageProvider messageProvider)
    {
        this.messageProvider = messageProvider;
    }

    protected override AccountCreateResponse Handle(AccountCreateRequest request)
    {
        //handle request
    }
}

I'm trying to get rid of the ServiceLocator in the base class. Is there a way to inject IConnectionProvider without having to change all derivative classes to include IConnectionProvider in their constructors and calling : base(connectionProvider)? I can't change DatabaseScopeManager.

Answer 1

The core of the problem here is the existence of your base class. You should get rid of the base class. Removing the base class will remove your problem completely.

The base class is problematic because:

• It causes every derived type to have a dependency on this class.
• It complicates testing, because all cross-cutting concerns are always executed.
• The base class will become a big endlessly growing class; a Single Responsibility Violation.

Although you can use Property Injection to get rid of the Service Locator, this doesn't remove the stated problems and even introduces a new problem, which is: Temporal Coupling.

Instead you should simply get rid of the base class and move these cross-cutting concerns into decorators. In your case I suggest creating two decorators.

A TransactionBusinessDecorator:

public class TransactionBusinessDecorator<TRequest, TResult>
    : IBusiness<TRequest, TResult>
    where TResult : BaseResponse
{
    private readonly IConnectionProvider connectionProvider;
    private readonly IBusiness<TRequest, TResult> decoratee;

    public TransactionBusinessDecorator(
        IConnectionProvider connectionProvider,
        IBusiness<TRequest, TResult> decoratee)
    {
        this.connectionProvider = connectionProvider;
        this.decoratee = decoratee;
    }

    public TResult Send(TRequest request)
    {
        TResult response;

        using (DatabaseScope scope = DatabaseScopeManager.Instance.GetScope(
            this.connectionProvider, 
            DatabaseScopeType.UseExistingOrNewTX,
            IsolationLevel.ReadCommitted))
        {
            response = this.decoratee.Handle(request);
            scope.Complete();
        }

        return response;        
    }
}

And a ExceptionWrapperBusinessDecorator:

public class ExceptionWrapperBusinessDecorator<TRequest, TResult>
    : IBusiness<TRequest, TResult>
    where TResult : BaseResponse
{
    private readonly IBusiness<TRequest, TResult> decoratee;

    public ExceptionWrapperBusinessDecorator(
        IBusiness<TRequest, TResult> decoratee)
    {
        this.decoratee = decoratee;
    }

    public TResult Send(TRequest request)
    {
        TResult response = Activator.CreateInstance<TResult>();

        try
        {
            var response = this.decoratee.Handle(request);
            response.Success = true;
        }
        catch (Exception ex)
        {
            response.Success = false;
            response.Message = exc.Message;
        }

        return response
    }
}

This allows the AccountCreateBusiness to be written as follows:

public sealed class AccountCreateBusiness 
    : IBusiness<AccountCreateRequest, AccountCreateResponse>
{
    private readonly IMessageProvider messageProvider;

    public AccountCreateBusiness(IMessageProvider messageProvider)
    {
        this.messageProvider = messageProvider;
    }

    public AccountCreateResponse Handle(AccountCreateRequest request)
    {
        //handle request
    }
}

Wrapping every handler with the decorators is trivial with most DI containers and when doing this by hand (a.k.a. Pure DI).

Was last design advice. Catching every exception and returning this information as part of the message is typically a bad idea, because:

• These error codes are leaking into the base of the system (your business layer) that has nothing to do with it. Instead of having this information available in the base response, use an envelope that contains this information.
• You will supply the client with technical information. The client is typically not interested in this level of detail. You only want to return function messages.
• Security sensitive information is returned; hackers love this kind of information.
• If you're not logging this information, you are losing a lot of important error details.
• To basically return error codes, instead of working with exceptions. Error codes are very error prone; the client needs to always check the result to see whether the response succeeded.

Answer 2

As Matías Fidemraizer mentioned, you may use Property Injection which is a quite safe way to avoid breaking changes. The drawback is that the property dependency in optional, but in your code the IConnectionProvider is a must.

In the long running perspective, I'd go with two constructors:

1. The default one which is still uses the Service Locator (for back compatibility) but is marked as [Obsolete] to indicate that it'll be removed in the next major version.
2. Constructor with the IConnectionProvider as a must-have dependency. This one should be recommended for use and should became the only one as soon as all the usages of the default constructor are eliminated.

Answer 3

There's an alternative: property injection.

BTW, property injection doesn't define mandatory dependencies, while constructor dependencies do it.