66

I am trying to adhere to the Single Responsibility Principle (SRP) as much as possible and got used to a certain pattern (for the SRP on methods) heavily relying on delegates. I'd like to know if this approach is sound or if there are any severe issues with it.

For example, to check input to a constructor, I could introduce the following method (the Stream input is random, could be anything)

private void CheckInput(Stream stream)
{
    if(stream == null)
    {
        throw new ArgumentNullException();
    }

    if(!stream.CanWrite)
    {
        throw new ArgumentException();
    }
}

This method (arguably) does more than one thing

  • Check the inputs
  • Throw different exceptions

To adhere to the SRP I therefore changed the logic to

private void CheckInput(Stream stream, 
                        params (Predicate<Stream> predicate, Action action)[] inputCheckers)
{
    foreach(var inputChecker in inputCheckers)
    {
        if(inputChecker.predicate(stream))
        {
            inputChecker.action();
        }
    }
}

Which supposedly only does one thing (does it?): Check the input. For the actual checking of the inputs and throwing of the exceptions I have introduced methods like

bool StreamIsNull(Stream s)
{
    return s == null;
}

bool StreamIsReadonly(Stream s)
{
    return !s.CanWrite;
}

void Throw<TException>() where TException : Exception, new()
{
    throw new TException();
}

and can call CheckInput like

CheckInput(stream,
    (this.StreamIsNull, this.Throw<ArgumentNullException>),
    (this.StreamIsReadonly, this.Throw<ArgumentException>))

Is this any better than the first option at all, or do I introduce unneccesary complexity? Is there any way I can still improve this pattern, if it's viable at all?

  • 25
    I could argue that CheckInput is still doing multiple things: It is both iterating over an array and calling a predicate function and calling a action function. Is that then not a violation of the SRP? – Bart van Ingen Schenau Oct 18 '17 at 9:27
  • 8
    Yes, that is the point I was trying to make. – Bart van Ingen Schenau Oct 18 '17 at 9:41
  • 133
    it's important to remember that it is the single responsibility principle; not the single action principle. It has one responsibility: verify the stream is defined and writable. – David Arno Oct 18 '17 at 10:32
  • 39
    Keep in mind that the whole point of these software principles is to make code more readable and maintainable. Your original CheckInput is far easier to read and maintain than your refactored version. In fact, if I ever came across your final CheckInput method in a codebase I'd scrap it all and rewrite it to match what you originally had. – 17 of 26 Oct 18 '17 at 13:49
  • 16
    These "principles" are practically useless because you can just define "single responsibility" in whichever way you want to go ahead with whatever your original idea was. But if you try to rigidly apply them I guess you end up with this kind of code which, to be frank, is hard to understand. – Casey Oct 18 '17 at 14:58

12 Answers 12

150

SRP is perhaps the most misunderstood software principle.

A software application is built from modules, which are built from modules, which are built from...

At the bottom, a single function such as CheckInput will only contain a tiny bit of logic, but as you go upward, each successive module encapsulates more and more logic and this is normal.

SRP is not about doing a single atomic action. It's about having a single responsibility, even if that responsibility requires multiple actions... and ultimately it's about maintenance and testability:

  • it promotes encapsulation (avoiding God Objects),
  • it promotes separation of concerns (avoiding rippling changes through the whole codebase),
  • it helps testability by narrowing the scope of responsibilities.

The fact that CheckInput is implemented with two checks and raise two different exceptions is irrelevant to some extent.

CheckInput has a narrow responsibility: ensuring that the input complies with the requirements. Yes, there are multiple requirements, but this does not mean there are multiple responsibilities. Yes, you could split the checks, but how would that help? At some point the checks must be listed in some way.

Let's compare:

Constructor(Stream stream) {
    CheckInput(stream);
    // ...
}

versus:

Constructor(Stream stream) {
    CheckInput(stream,
        (this.StreamIsNull, this.Throw<ArgumentNullException>),
        (this.StreamIsReadonly, this.Throw<ArgumentException>));
    // ...
}

Now, CheckInput does less... but its caller does more!

You have shifted the list of requirements from CheckInput, where they are encapsulated, to Constructor where they are visible.

Is it a good change? It depends:

  • If CheckInput is only called there: it's debatable, on the one hand it makes the requirements visible, on the other hand it clutters the code;
  • If CheckInput is called multiple times with the same requirements, then it violates DRY and you have an encapsulation issue.

It's important to realize that a single responsibility may imply a lot of work. The "brain" of a self-driving car has a single responsibility:

Driving the car to its destination.

It is a single responsibility, but requires coordinating a ton of sensors and actors, taking lots of decision, and even has possibly conflicting requirements1...

... however, it's all encapsulated. So the client doesn't care.

1 safety of the passengers, safety of others, respect of regulations, ...

  • 2
    I think the way you are using the word "encapsulation" and its derivatives is confusing. Other than that, great answer! – Fabio Turati Oct 18 '17 at 13:02
  • 4
    I agree with your answer, but the self-driving car brain argument often tempt people to break SRP. Like you said, it's modules made of modules made of modules. You can identify the purpose of that whole system, but that system should be broken up its self. You can break down almost any problem. – Sava B. Oct 18 '17 at 14:45
  • 13
    @SavaB.: Sure, but the principle remains the same. A module should have a single responsibility, although of larger scope than its constituents. – Matthieu M. Oct 18 '17 at 16:50
  • 3
    @user949300 Okay, how about just "driving." Really, "driving" is the responsibility and "safely" and "legally" are requirements about how it fulfills that responsibility. And we often list the requirements when stating a responsibility. – Brian McCutchon Oct 19 '17 at 4:37
  • 1
    "SRP is perhaps the most misunderstood software principle." As evidenced by this answer :) – Michael Oct 19 '17 at 8:56
41

Quoting Uncle Bob about the SRP (https://8thlight.com/blog/uncle-bob/2014/05/08/SingleReponsibilityPrinciple.html):

The Single Responsibility Principle (SRP) states that each software module should have one and only one reason to change.

... This principle is about people.

... When you write a software module, you want to make sure that when changes are requested, those changes can only originate from a single person, or rather, a single tightly coupled group of people representing a single narrowly defined business function.

... This is the reason we do not put SQL in JSPs. This is the reason we do not generate HTML in the modules that compute results. This is the reason that business rules should not know the database schema. This is the reason we separate concerns.

He explains that software modules must address specific stakeholders worries. Therefore, answering your question:

Is this any better than the first option at all, or do I introduce unneccesary complexity? Is there any way I can still improve this pattern, if it's viable at all?

IMO, you're only looking at one method, when you should look at a higher level (class level in this case). Perhaps we should take a look on what your class is currently doing (and this requires more explanation about your scenario). For now, your class is still doing the same thing. For instance, if tomorrow there are some change request about some validation (eg: "now the stream can be null"), then you still need to go to this class and change stuff within it.

  • 4
    Best answer. To elaborate regarding OP, if the guard checks come from two different stakeholders / departments, then checkInputs() should be split, say into checkMarketingInputs() and checkRegulatoryInputs(). Otherwise it is fine to combine them all into one method. – user949300 Oct 19 '17 at 4:24
36

No, this change is not informed by the SRP.

Ask yourself why there is no check in your checker for "the object passed in is a stream". The answer is obvious: the language prevents the caller from compiling a program that passes in a non-stream.

The type system of C# is insufficient to meet your needs; your checks are implementing enforcement of invariants that cannot be expressed in the type system today. If there was a way to say that the method takes a non-nullable writeable stream, you'd have written that, but there isn't, so you did the next best thing: you enforced the type restriction at runtime. Hopefully you also documented it, so that developers who use your method do not have to violate it, fail their test cases, and then fix the problem.

Putting types on a method is not a violation of the Single Responsibility Principle; neither is the method enforcing its preconditions or asserting its postconditions.

  • 1
    Also, leaving the created object in a valid state is the one responsibility a constructor fundamentally always has. If it, like you mentioned, requires additional checks that the runtime and/or compiler cannot provide, then there really isn't a way around it. – SBI Oct 20 '17 at 8:13
22

Not all responsibilities are created equal.

enter image description here

enter image description here

Here are two drawers. They both have one responsibility. They each have names that let you know what belongs in them. One is the silverware drawer. The other is the junk drawer.

So what's the difference? The silverware drawer makes clear what doesn't belong in it. The junk drawer however accepts anything that will fit. Taking the spoons out of the silverware drawer seems very wrong. Yet I'm hard pressed to think of anything that would be missed if removed from the junk drawer. The truth is you can claim anything has a single responsibility but which do you think has the more focused single responsibility?

An object having a single responsibility doesn't mean only one thing can happen in here. Responsibilities can nest. But those nesting responsibilities should make sense, they shouldn't surprise you when you find them in here and you should miss them if they were gone.

So when you offer

CheckInput(Stream stream);

I don't find myself concerned that it's both checking input and throwing exceptions. I would be concerned if it was both checking input and saving input as well. That's a nasty surprise. One I wouldn't miss if it was gone.

21

When you tie yourself in knots and write weird code in order to conform to an Important Software Principle, usually you've misunderstood the principle (though sometimes the principle is wrong). As Matthieu's excellent answer points out, the entire meaning of SRP depends on the definition of "responsibility".

Experienced programmers see these principles and relate them to memories of code we screwed up; less experienced programmers see them and may have nothing to relate them to at all. It's an abstraction floating in space, all grin and no cat. So they guess, and it usually goes badly. Before you've developed programming horse sense, the difference between weird overcomplicated code and normal code is not at all obvious.

This isn't a religious commandment that you must obey regardless of personal consequences. It's more of a rule of thumb meant to formalize one element of programming horse sense, and help you keep your code as simple and clear as possible. If it's having the opposite effect, you're right to look for some outside input.

In programming, you can't go much wronger than trying to deduce the meaning of an identifier from first principles by just staring at it, and that goes for identifiers in writing about programming just as much as identifiers in actual code.

14

CheckInput role

First, let me put the obvious out there, CheckInput is doing one thing, even if it is checking various aspects. Ultimately it checks input. One could argue that it isn't one thing if you're dealing with methods called DoSomething, but I think it is safe to assume that checking input is not too vague.

Adding this pattern for predicates could be useful if you want don't want the logic for checking the input to be placed into your class, but this pattern seems rather verbose for what you're trying to achieve. It might be far more direct to simply pass an interface IStreamValidator with single method isValid(Stream) if that is what you wish to obtain. Any class implementing IStreamValidator can use predicates like StreamIsNull or StreamIsReadonly if they wish, but getting back to the central point, it is a rather ridiculous change to make in the interests of maintaining the single responsibility principle.

Sanity check

It is my idea that we're all allowed a "sanity check" to ensure that you're at least dealing with a Stream that is non-null and writeable, and this basic check is not somehow making your class a validator of streams. Mind you, more sophisticated checks would be best left outside your class, but that's where the line is drawn. Once you need to begin changing state of your stream by reading from it or dedicating resources towards validation, you've started performing a formal validation of your stream and this is what should be pulled into its own class.

Conclusion

My thoughts are that if you're applying a pattern to better organize an aspect of your class, it merits to be in its own class. Since a pattern doesn't fit, you should also question whether or not it really does belong in its own class in the first place. My thoughts are that unless you believe validation of the stream are likely going to be changed in the future, and especially if you believe this validation may even likely be dynamic in nature, then the pattern you described is a good idea, even if it may be initially trivial. Otherwise, there is no need to arbitrarily make your program more complex. Lets call a spade a spade. Validation is one thing, but checking for null input isn't validation, and therefore I think you can be safe to keep it in your class without violating the single responsibility principle.

4

The principle emphatically does not state a piece of code should "only do a single thing".

"Responsibility" in SRP should be understood at a the requirements level. The responsibility of code is to satisfy business requirements. SRP is violated if an object satisfies more than one independent business requirements. By independent it means that one requirement could change while the other requirement stays in place.

It is conceivable that a new business requirement is introduced which means this particular object shouldn't check for readable, while another business requirement still requires the object to check for readable? No, because business requirements does not specify implementation details at that level.

An actual example of a SRP violation would be code like this:

var message = "Your package will arrive before " + DateTime.Now.AddDays(14);

This code is very simple, but still it is conceivable that the text will change independently from the expected delivery date, since these are decided by different parts of the business.

  • A different class for virtually every requirement sounds like an unholy nightmare. – whatsisname Oct 23 '17 at 4:33
  • @whatsisname: Then perhaps the SRP is not for you. No design principle applies for all kinds and sizes of projects. (But be aware we are only talking of independent requirements (i.e. can change independently), not just any requirement since then it would just depend on how fine-grained they are specified.) – JacquesB Oct 23 '17 at 8:25
  • I think it's more that the SRP requires an element of situational judgement which is challenging to describe in a single catchy phrase. – whatsisname Oct 23 '17 at 14:34
  • @whatsisname: I totally agree. – JacquesB Oct 23 '17 at 15:48
  • +1 for SRP is violated if an object satisfies more than one independent business requirements. By independent it means that one requirement could change while the other requirement stays in place – Juzer Ali Feb 7 '18 at 13:22
3

I like the point from @EricLippert's answer:

Ask yourself why there is no check in your checker for the object passed in is a stream. The answer is obvious: the language prevents the caller from compiling a program that passes in a non-stream.

The type system of C# is insufficient to meet your needs; your checks are implementing enforcement of invariants that cannot be expressed in the type system today. If there was a way to say that the method takes a non-nullable writeable stream, you'd have written that, but there isn't, so you did the next best thing: you enforced the type restriction at runtime. Hopefully you also documented it, so that developers who use your method do not have to violate it, fail their test cases, and then fix the problem.

EricLippert's right that this is an issue for the type system. And since you want to use the single-responsibility principle (SRP), then you basically need the type system to be responsible for this job.

It's actually possible to sorta do this in C#. We can catch literal null's at compile time, then catch non-literal null's at run-time. That's not as good as a full compile-time check, but it's a strict improvement over never catching at compile-time.

So, ya know how C# has Nullable<T>? Let's reverse that and make a NonNullable<T>:

public struct NonNullable<T> where T : class
{
    public T Value { get; private set; }
    public NonNullable(T value)
    {
        if (value == null) { throw new NullArgumentException(); }
        this.Value = value;
    }
    //  Ease-of-use:
    public static implicit operator T(NonNullable<T> value) { return value.Value; }
    public static implicit operator NonNullable<T>(T value) { return new NonNullable<T>(value); }

    //  Hack-ish overloads that prevent null-literals from being implicitly converted into NonNullable<T>'s.
    public static implicit operator NonNullable<T>(Tuple<T> value) { return new NonNullable<T>(value.Item1); }
    public static implicit operator NonNullable<T>(Tuple<T, T> value) { return new NonNullable<T>(value.Item1); }
}

Now, instead of writing

public void Foo(Stream stream)
{
  if (stream == null) { throw new NullArgumentException(); }

  // ...method code...
}

, just write:

public void Foo(NonNullable<Stream> stream)
{
  // ...method code...
}

Then, there're three use-cases:

  1. User calls Foo() with a non-null Stream:

    Stream stream = new Stream();
    Foo(stream);
    

    This is the desired use-case, and it works with-or-without NonNullable<>.

  2. User calls Foo() with a null Stream:

    Stream stream = null;
    Foo(stream);
    

    This is a calling error. Here NonNullable<> helps inform the user that they shouldn't be doing this, but it doesn't actually stop them. Either way, this results in a run-time NullArgumentException.

  3. User calls Foo() with null:

    Foo(null);
    

    null won't implicitly convert into a NonNullable<>, so the user gets an error in the IDE, before run-time. This is delegating the null-checking to the type system, just as the SRP would advise.

You can extend this method to assert other things about your arguments, too. For example, since you want a writable stream, you can define a struct WriteableStream<T> where T:Stream that checks for both null and stream.CanWrite in the constructor. This would still be a run-time type check, but:

  1. It decorates the type with the WriteableStream qualifier, signaling the need to callers.

  2. It does the check in a single place in code, so you don't have to repeat the check and throw InvalidArgumentException each time.

  3. It better conforms to the SRP by pushing the type-checking duties to the type system (as extended by the generic decorators).

3

Your approach is currently procedural. You're breaking apart the Stream object and validating it from the outside. Don't do that - it breaks encapsulation. Let the Stream be responsible for its own validation. We can't seek to apply the SRP until we've got some classes to apply it to.

Here's a Stream which performs an action only if it passes validation:

class Stream
{
    public void someAction()
    {
        if(!stream.canWrite)
        {
            throw new ArgumentException();
        }

        System.out.println("My action");
    }
}

But now we're violating SRP! "A class should have only one reason to change." We've got a mix of 1) validation and 2) actual logic. We've got two reasons it might need to change.

We can solve this with validating decorators. First, we need to convert our Stream to an interface and implement that as a concrete class.

interface Stream
{
    void someAction();
}

class DefaultStream implements Stream
{
    @Override
    public void someAction()
    {
        System.out.println("My action");
    }
}

We can now write a decorator which wraps a Stream, performs validation and defers to the given Stream for the actual logic of the action.

class WritableStream implements Stream
{
    private final Stream stream;

    public WritableStream(final Stream stream)
    {
        this.stream = stream;
    }

    @Override
    public void someAction()
    {
        if(!stream.canWrite)
        {
            throw new ArgumentException();
        }
        stream.someAction();
    }
}

We can now compose these any way we like:

final Stream myStream = new WritableStream(
    new DefaultStream()
);

Want additional validation? Add another decorator.

1

A class's job is provide a service that meets a contract. A class always has a contract: a set of requirements for using it, and promises it makes about its state and outputs provided that the requirements are met. This contract may be explicit, through documentation and/or assertions, or implicit, but it always exists.

Part of your class's contract is that the caller give the constructor some arguments that must not be null. Implementing the contract is the class's responsibility, so to check that the caller has met its part of the contract can easily be considered to be within the scope of the class's responsibility.

The idea that a class implements a contract is due to Bertrand Meyer, the designer of the Eiffel programming language and of the idea of design by contract. The Eiffel language makes the specification and checking of the contract part of the language.

0

As has been pointed out in other answers SRP is often misunderstood. It's not about having atomic code that only does one function. It's about making sure that your objects and methods only do one thing, and that the one thing is only done in one place.

Lets look at a poor example in pseudo code.

class Math
    private int a;
    private int b;
    def constructor(int x, int y) 
        if(x != null)
          a = x
        else if(x < 0)
          a = abs(x)
        else if (x == -1)
          throw "Some Silly Error"
        else
          a = 0
        end
        if(y != null)
           b = y
        else if(y < 0)
           b = abs(y)
        else if(y == -1)
           throw "Some Silly Error"
        else
         b = 0
        end
    end
    def add()
        return a + b
    end
    def sub()
        return b - a
    end
end

In our rather absurd example, the "responsibility" of Math#constructor is to make the math object usable. It does so by first sanitizing the input, then by making sure the values are not -1.

This is valid SRP because the constructor is doing only one thing. It's preparing the Math object. However it's not very maintainable. It violates DRY.

So lets take another pass at it

class Math
    private int a;
    private int b;
    def constructor(int x, int y)
        cleanX(x)
        cleanY(y)
    end
    def cleanX(int x)
        if(x != null)
          a = x
        else if(x < 0)
          a = abs(x)
        else if (x == -1)
          throw "Some Silly Error"
        else
          a = 0
        end
   end
   def cleanY(int y)
        if(y != null)
           b = y
        else if(y < 0)
           b = abs(y)
        else if(y == -1)
           throw "Some Silly Error"
        else
         b = 0
        end
    end
    def add()
        return a + b
    end
    def sub()
        return b - a
    end
end

In this pass we got a bit better about DRY, but we still have a ways to go with DRY. SRP on the other hand seems a bit off. We now have two functions with the same job. Both cleanX and cleanY sanitize input.

Lets give it another go

class Math
    private int a;
    private int b;
    def constructor(int x, int y)
        a = clean(x)
        b = clean(y)
    end
    def clean(int i)
        if(i != null)
          return i
        else if(i < 0)
          return abs(i)
        else if (i == -1)
          throw "Some Silly Error"
        else
          return 0
        end
    end
    def add()
        return a + b
    end
    def sub()
        return b - a
    end
end

Now finally were better about DRY, and SRP seems to be in agreement. We have only one place that does the "sanitize" job.

The code is in theory more maintainable and better yet when we go to fix the bug and tighten up the code, we only need to do it in one place.

class Math
    private int a;
    private int b;
    def constructor(int x, int y)
        a = clean(x)
        b = clean(y)
    end
    def clean(int i)
        if(i == null)
          return 0
        else if (i == -1)
          throw "Some Silly Error"
        else
          return abs(i)
        end
    end
    def add()
        return a + b
    end
    def sub()
        return b - a
    end
end

In most real world cases the objects would be more complex and SRP would be applied across a bunch of objects. For example age may belong to Father, Mother, Son, Daughter, so instead of having 4 classes that figure out age from the date of birth you have a Person class that does that and the 4 classes inherit from that. But I hope this example helps to explain. SRP is not about atomic actions, but about a "job" getting done.

-3

Speaking of SRP, Uncle Bob doesn't like null checks sprinkled everywhere. In general you, as a team, should avoid using null parameters to constructors whenever possible. When you publish your code outside of your team things may change.

Enforcing non-nullability of constructor parameters without first assuring the cohesiveness of the class in question results in bloat in the calling code, especially tests.

If you really want to enforce such contracts consider using Debug.Assert or something similar to reduce clutter:

public AClassThatDefinitelyNeedsAWritableStream(Stream stream)
{
   Assert.That(stream.CanWrite, "Put crucial information here, and not inane bloat.");

   // Go on normal operation.
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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