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?

I would say "No" if you set it's responsibility to "shipping orders" — Sasha, Apr 10 '13 at 12:29

score 2 · Accepted Answer · answered Apr 10 '13 at 12:56

2

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

answered Apr 10 '13 at 12:56

Alex

7,639
3
45
58

1

And don't forget you can add crosscutting concerns using good old decorators as well (as [this article](http://www.cuttingedge.it/blogs/steven/pivot/entry.php?id=91) shows). – Steven Apr 10 '13 at 13:05
MikeSW states in his answer that my code does violate SRP. What do you think about is arguments? – user782596 Apr 10 '13 at 13:33
It depends on how you look at it. I think the principle can be taken too far - in my eyes, you have not violated SRP because the fundamental cause for change in your class is the business logic of shipping an order. Other responsibilities are delegated elsewhere. The only place I can suggest improvement here is by using factories to create your `Profiler` and `Transaction` objects - then you are only bound to an interface for your external dependencies, which is fine (it is assumed that interfaces will not change) – Alex Apr 10 '13 at 13:43

score 1 · Answer 2 · answered Apr 10 '13 at 12:30

1

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.

answered Apr 10 '13 at 12:30

Claudio Redi

67,454
15
130
155

MikeSW claims that the code actually breaks SRP. What do you think about his statement? – user782596 Apr 10 '13 at 13:33

score 1 · Answer 3 · answered Apr 10 '13 at 12:37

1

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.

answered Apr 10 '13 at 12:37

jgauffin

99,844
45
235
372

You didn't answer my question. Does my code violate SRP or not? – user782596 Apr 10 '13 at 12:48
`That's pretty solid` was my answer. i.e. yes your code is solid (it just need some love). – jgauffin Apr 10 '13 at 12:53
I don't get it how the OP code is SOLID. Your solution may resemble to, but for me, it's clearly different approach than OP's – MikeSW Apr 10 '13 at 13:00
I don't my code is SOLID. It violates the OCP, since the code has to be changed every time a new crosscutting concern is added. But if I understand correctly, you think it doesn't violate SRP. – user782596 Apr 10 '13 at 13:03
1

you're right. My code is solid, but the OP's is just SRP. OCP/LSP doesn't work very well with static classes. – jgauffin Apr 10 '13 at 13:04
Besides, the OP's code does also violate the DIP, which your code also solves. – Steven Apr 10 '13 at 13:09

score 1 · Answer 4 · answered Apr 10 '13 at 12:56

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

You are currently the only one who states that my code actually violates SRP. Can you back this up with some proof? Perhaps by quoting the definition or some other reference? — user782596, Apr 10 '13 at 13:15
Lol, definition and references... What is this class supposed to do? Ship orders? Then that's its responsibility. If it handles other concerns besides shipping orders, then you have more than one single responsibility. For example, this class should never handle user authorization. About profiling, I don't have enough information, but I'm very sure that the fact you're using static class with static methods has a big influence. — MikeSW, Apr 10 '13 at 13:27

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

4 Answers4