Does a constructor that validates its arguments violate SRP?

https://softwareengineering.stackexchange.com/questions/359320

21-01-2021
|

Question

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?

Solution

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 requirements¹...

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

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

OTHER TIPS

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.

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.

Not all responsibilities are created equal.

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.

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.

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.

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.

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:

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<>.
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.
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:

It decorates the type with the WriteableStream qualifier, signaling the need to callers.
It does the check in a single place in code, so you don't have to repeat the check and throw InvalidArgumentException each time.
It better conforms to the SRP by pushing the type-checking duties to the type system (as extended by the generic decorators).

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.

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.

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.

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.
}

Licensed under: CC-BY-SA with attribution

Not affiliated with softwareengineering.stackexchange