C# Design Principles – Does a Constructor That Validates Its Arguments Violate SRP?

csingle-responsibility

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?

Best Answer

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

Related Topic