Is calling into other code a (SOLID) Single Responsibility Principles (SRP) violation?

Question

Considering this class with business logic:

public static class OrderShipper
{
    public static void ShipOrder(Order order) {
        AuthorizationHelper.AuthorizedUser();

        using (new PerformanceProfiler()) {
            OperationRetryHelper.HandleWithRetries(() => ShipOrderInTransaction(order));
        }
    }

    private static void ShipOrderInTransaction(Order order) {
        using (var transaction = new TransactionHelper()) {
            ShipOrderInternal(order);

            transaction.Commit();
        }            
    }

    private static void ShipOrderInternal(order) {
        // lots of business logic
    }
}

The class contains some business logic, and executes some crosscutting concerns as well. Although there is no doubt about that this class violates the Open/Closed Principle, does this class violate the Single Responsibility Principle?

I'm in doubt, since the class itself is not responsible for authorizing the user, for profiling the performance and for handling the transaction.

There is no question about this that this is poor design, since the class is still (statically) depending on those crosscutting concerns, but still: Is it violating the SRP. If so, why is this?

Answer 1

It's a good question, the title is slightly misleading (it's unlikely you can build an application without "calling into other code"). Remember that the SOLID principles are more guidelines than absolute rules that must be followed; if you take SRP to its logical conclusion, you will end up with one method per class. The way to minimise the impact of cross-cutting concerns is to create a facade that is as easy as possible to use. In your examples you have done this well - each crosscutting concern only uses one line.

Another way to achieve this is through AOP, which is possible in C# by using PostSharp or through IoC interception

Answer 2

There is nothing wrong with a class method coordinating some other classes activities, that's not breaking the SRP. You will break it if the logic of those classes was part of OrderShipper.

I'm not sure what PerformanceProfiler does, but is the only component that looks weird in there.

Answer 3

Let's make it more visible by converting the class to a command:

// Command pattern
public class ShipOrder
{
    ITransactionFactory _transactionFactory;


    public OrderShipper(ITransactionFactory factory)
    {
        if (factory == null) throw new ArgumentNullException("factory");

        _transactionFactory = factory;
    }

    [PrincipalPermission(Roles = "User")]
    public void Execute(Order order) 
    {
        if (order == null) throw new ArgumentNullException("order");

        using (new PerformanceProfiler()) 
        {
            HandleWithRetries(() => ShipOrderInTransaction(order));
        }
    }

    private void ShipOrderInTransaction(Order order) 
    {
        using (var transaction = _transactionFactory.Create()) 
        {
            ShipOrderInternal(order);

            transaction.Commit();
        }            
    }

    protected void ShipOrderInternal(order) 
    {
        // bussiness logic which is divided into different protected methods.
    }
}

Hence you can call it using:

var cmd = new ShipOrder(transactionFactory);
cmd.Execute(order);

That's pretty solid.

Answer 4

Yes, it does break SRP, at least according to the class name.

the class itself is not responsible for authorizing the user, for profiling the performance and for handling the transaction.

You are answering yourself,it should contain only the shipping order logic. And it shouldn't be static (why is it static?!).

The solution provided by @jgauffin is a posibility, although I'm not entirely convinced that the OrderShipper should know about a transaction or it should be just a part of one. ALso, the performance profiler, IMO, has no place in this class. But having only this information I can't suggest a solution. The profiling though is a crosscuting concern and it might be better to be handled outside of this class, perhaps with an attribute.

Btw, using a message driven approach (as suggested by jgauffin) it should allow the infrastructure to provide profiling and reliability (HandleWithRetries) support