15

I'm trying to understand the SRP but, whilst I understand the reasoning behind how to apply it, I'm not really seeing the benefit of doing so. Consider this example, taken from Robert Martin's SRP PDF:

interface IModem
{
    void Dial(string number);
    void Hangup();
    void Send(char c);
    char Recv();
}

He proposes separating this into two interfaces:

interface IModemConnection
{
    void Dial(string number);
    void Hangup();
}

interface IModemDataExchange
{
    void Send(char c);
    char Recv();
}

I've also been reading this article, which takes this one step further:

interface IModemConnection : IDisposable
{
    IModemDataExchange Dial(string number);
}

interface IModemDataExchange
{
    void Send(char c);
    char Recv();
}

At this point, I understand what is meant by functional (Send / Recv) and non-functional (Dial / Hangup) aspects, but I don't see the benefit of separating them in this example. Considering this basic implementation:

class ConcreteModem : IModemConnection
{
    public IModemDataExchange Dial(string number)
    {
        if (connection is successful)
        {
            return new ConcreteModemDataExchange();
        }

        return null;
    }

    public void Dispose()
    {
        // 
    }

    public bool IsConnected { get; private set; }
}

At this point, let me quote Robert Martin again, even though he's talking about a different example from that PDF:

Secondly, if a change to the GraphicalApplication causes the Rectangle to change for some reason, that change may force us to rebuild, retest, and redeploy the ComputationalGeometryApplication. If we forget to do this, that application may break in unpredictable ways.

This is what I don't understand. If I had to create a second implementation of IModemDataExchange, and I wanted to make use of that, I would still have to change the Dial method, meaning the class also needs to be recompiled:

public IModemDataExchange Dial(string number)
{
    if (some condition is met)
    {
        return new ConcreteModemDataExchange();
    }
    else if (another condition is met)
    {
        return new AnotherConcreteModemDataExchange();
    }

    return null;
}

I can't see what this has done to reduce the effects of change on the class. It still needs to be recompiled, so what's the benefit? What do you gain from doing this that is so important to producing quality code?

Dave Schweisguth
  • 36,475
  • 10
  • 98
  • 121
John H
  • 14,422
  • 4
  • 41
  • 74
  • I'm having trouble joining the dots between the quote and the example code. The quote seems to reference 2 different applications with a shared codebase. – Gusdor Nov 19 '13 at 13:44
  • @Gusdor It does reference two different applications, but from everything I've read about SRP so far, it seems the idea is to improve flexibility of your codebase, whilst reducing the effects of change. I highlighted that particular quote because I don't see how going through these steps reduces the effects of change at all. – John H Nov 19 '13 at 13:46
  • 1
    After researching Patrick's answer, I came across [this question](http://programmers.stackexchange.com/questions/155852/programming-solid-principles) on PSE, which should help anyone else who finds this question. – John H Nov 19 '13 at 22:46

4 Answers4

10

To me the modem example above always seemed like a case for the interface segregation principle rather than SRP, but that's besides the point.

In the part you called out regarding the Rectangle, I think you're just misinterpreting it. Martin is using the Rectangle as an example of a shared library. If the GraphicalApplication requires a new method or change in semantics in the Rectangle class, then that affects the ComputationalGeometryApplication since they both "link" to the Rectangle library. He's saying it violates SRP because it is responsible for defining rendering bounds and also an algebraic concept. Imagine if the GraphicalApplication changed from DirectX to OpenGL where the y-coordinate is inverted. You may want to change some things around on the Rectangle to facilitate this, but you're then potentially causing changes in ComputationalGeometryApplication.

In my work, I try to follow the SOLID principles and TDD, and I've found that SRP makes writing tests for classes simple and also keeps the classes focused. Classes that follow SRP are typically very small, and this reduces complexity both in code and dependencies. When stubbing out classes, I try to make sure a class is either "doing one thing", or "coordinating two (or more) things". This keeps them focused and makes their reasons for change only dependent on the one thing they do, which to me is the point of SRP.

Patrick Quirk
  • 23,334
  • 2
  • 57
  • 88
  • Thanks a lot for this answer, Patrick. This cleared up quite a lot for me, and it probably explains why that example didn't make complete sense to me. I do have one question regarding ISP though. Let's say an application has already established its own connection, would I be right in saying the idea is that it shouldn't have to implement the whole of `IModem` (which I guess, sort of goes against DRY), when it really only needs the `Send` and `Recv` methods? – John H Nov 19 '13 at 15:04
  • 1
    @JohnH: To me, ISP is all about hiding complexity and keeping changes contained (which is where it blends with SRP IMHO). If you only need `Send` and `Recv`, then that means you create an interface named something like `IDataExchange` that only has those two methods, and this even hides the fact that it's a modem to maintain abstractions. If the signature to `Hangup` had to change, you won't have to recompile objects that only use `IDataExchange`, which isn't true for any class that uses `IModem`, whether they use `Hangup` or not. – Patrick Quirk Nov 19 '13 at 15:18
3

The main benefit is quite obvious. By splitting up you're giving your model better logical grouping which in turn makes the intention clearer and maintenance easier.

If I had to create a second implementation of IModemDataExchange, and I wanted to make use of that, I would still have to change the Dial method

Yes will have to, but that is not the benefit there. One benefit is, when you have any modification for IModemDataExchange interface itself you only have to change concrete implementations of the interface, not ConcreteModem itself which will make maintenance of subscribers of Dial method easier. Another benefit is that now even if you have to write an additional IModemDataExchange implementation, then changes it will require in ConcreteModem class is minimized, there isn't a direct coupling. By separating responsibilities you minimize the side effects of modifications.

Not requiring to recompile is not the essence here. And in a strict sense, what if one of those interface is in another project? It saves recompilation of one project. The stress is on not requiring to change code at lot of places. Of course any change would require a recompilation.

nawfal
  • 70,104
  • 56
  • 326
  • 368
  • Thanks for your answer, nawfal. It was worth reading just for this: `By splitting up you're giving your model better logical grouping which in turn makes the intention clearer.`. I'd unconsciously noticed it, but not really acknowledged it properly. However, your example didn't make any sense to me. The original `IModem` interface doesn't return `IModem` from `Dial`. – John H Nov 19 '13 at 15:01
  • @JohnH I completely missed it. Let me edit that part. – nawfal Nov 19 '13 at 21:40
  • Thanks a lot for the edit. That's much clearer now. – John H Nov 19 '13 at 22:44
1

You don't need to change ConcreteModem if you use an abstract factory. Or if you parameterize generic Modem<TModemDataExchange> (or generic Dial<TModemDataExchange>() method) by a concrete type that should be created on success.

The idea is that IModemConnection implementation doesn't depend on any info about IModeDataExchange implementation except its name.

Moving forward, I'd consider following approach:

interface IModemConnection : IDisposable
{
    void Dial(string number);
}

interface IModemDataExchange
{
    void Send(char c);
    char Recv();
}

class ConcreteModemDataExchange : IModemDataExchange
{
    ConcreteModemDataExchange(IModemDataExchange);
}

So to create ConcreteModemDataExchange instance you need to have a connection. There's still a possibility to have disconnected instance of connection, but it's a different story.

As a side node, I'd recommend to throw an exception in Dial on failure.

Andriy Tylychko
  • 15,967
  • 6
  • 64
  • 112
  • Thanks for the example, Andy. I take it you meant `ConcreteModemDataExchange(IModemConnection);` though? As for the side note, would you mind explaining why it should throw an exception ? I was actually wondering about this when I posted it, funnily enough. – John H Nov 19 '13 at 14:58
  • 1
    @JohnH: from logical point of view: because it failed to accomplish its goal, from technical point of view: you won't forget to check for null – Andriy Tylychko Nov 19 '13 at 15:10
1

I don't know a lot about how modems work, so I struggled a bit to come up with a meaningful example. However, consider this:

Having separated the logic for dialing, now if some other parts of the program only need to dial, we can pass in just a IModemConnection. This could be useful even in a Modem class itself, using dependency injection:

public class Modem : IModemConnection, IModemDataExchange
{
    public IModemConnection Dialer {get; private set;}

    public Modem(IModemConnection Dialer)
    {
        this.Dialer=Dialer;
    }

    public void Dial(string number)
    {
        Dialer.Dial(number);
    }

    public void Hangup()
    {
        Dialer.Hangup();
    } 

    // .... implement IModemDataExchange
}

Now you could have:

public class DigitalDialer : IModemConnection
{
    public void Dial(string number)
    {
        Console.WriteLine("beep beep");
    }
    public void Hangup()
    {
        //hangup
    }
}

and

public class AnalogDialer : IModemConnection
{
    public void Dial(string number)
    {
        Console.WriteLine("do you even remember these?");
    }
    public void Hangup()
    {
        //hangup
    }
}

now, if you want to change some aspects of how your modem works (the way it dials a number in this case) your changes will be localized in a Dialer class that has a single responsibility (dialing).

Paolo Falabella
  • 24,914
  • 3
  • 72
  • 86
  • Thanks for this example, Paolo - it reassures me that I'm heading in the right direction. I came up with several implementations of the class, one of which was something a lot like this. I honestly thought I was missing something with the original implementation, which is why I posted it. I really appreciate the help. – John H Nov 19 '13 at 14:56