SRP violation confusion based on examples

Question

I am trying to gain better understanding on SRP and when I was checking out a Pluralsight video by Scott Allen, I saw code like this:

https://github.com/OdeToCode/cs-fundamentals/blob/master/src/GradeBook/Book.cs

https://github.com/OdeToCode/cs-fundamentals/blob/master/src/GradeBook/Statistics.cs

public class DiskBook : Book
    {
        public DiskBook(string name) : base(name)
        {
        }

        public override event GradeAddedDelegate GradeAdded;

        public override void AddGrade(double grade)
        {
            using(var writer = File.AppendText($"{Name}.txt"))
            {                
                writer.WriteLine(grade);
                if(GradeAdded != null)
                {
                    GradeAdded(this, new EventArgs());
                }
            }
        }

        public override Statistics GetStatistics()
        {
            var result = new Statistics();

            using(var reader = File.OpenText($"{Name}.txt"))
            {
                var line = reader.ReadLine();
                while(line != null)
                {
                    var number = double.Parse(line);
                    result.Add(number);
                    line = reader.ReadLine();
                }
            }

            return result;
        }
    }

Now here Diskbook is responsible to manage grades and Statistics class responsibility is to calculate statistics based on grades but this makes me wonder that having GetStatistics method in DiskBook class doesnt break SRP?

Because as per my understanding if there will be some changes inside Statistics class, then I will have to change logic in GetStatistics method which means now I have 2 reasons to change DiskBook class. Clear SRP violation?

Also in .net framework we have DbConnection class which creates DbCommand like below:

public abstract class DbConnection : Component, IDbConnection, IDisposable
{
    protected abstract DbCommand CreateDbCommand();
}

I know this won't be violating SRP since it is designed and created by .net team but I would like to understand why this is not violating SRP and when does it make sense to do something like this?

Answer 1

SRP is a principle, not a Law.

You follow it because generally it leads to better code. Code that breaks it, is not by definition bad. In fact there is a lot of code out there that is arguably good, that does not follow this principle.

To put this into context: it is a Law that code must compile. Code that does not compile is by definition very bad code. Its not code at all.

---

That all said. The snippet of code you gave both does and does not break the principle. It is about perspective.

• It breaks the principle because Statistics and Grades are at two different things. You can see this because Grades have to processed to make Statistics. That process can change independently of the Grades themselves.
• It follows the principle because Statistics and Grades are highly correlated. Chances are that a change will likely affect both Grades and Statistics. These concepts are highly entwined in many academic systems. Practices such as grading to a curve, produce an interplay between both of them. Other practices such as how many grades (A-D, A-E, A-F, etc...) are entwined with how they are processed into statistics.

Whether this class follows SRP will only be dictated by future changes. At which point if it doesn't it would probably make better code to split them apart.

Either way, right now the code is legible. Which is a much more important to code quality. You will read the same code a lot.

So I would leave it be. Later you will know better.

---

Again with the DbConnection generating a DbCommand this is both SRP and not SRP.

• It is SRP because the DBConnection is the guardian of the sacred db resource and will only permit access by trusted implementations of DbCommand. As such it is its responsibility to enforce this.

• It is not SRP because what does a class representing a communication socket care about how a query is constructed or its result represented. Surely it accepts some sequence of instructs and presents this to the database on the other end. Any results can be heard by reading the responses from the server.

The tipping point for the .Net team was not SRP, but instead plugin architecture. Plugin Architecture is very flexible and allows many different implementations (in this case different database engines) to be used by the software. The downside is that this is dynamic and identifying which plugin to use in a specific case can be very heavy weight. So you want to limit how many times you need to pick which plugin to use.

In .net identifying the right db plugin to use is done by using a connection string, which specifies a protocol. Each known provider is asked if it can understand the string. One of those that can understand it is picked, and a connection object of the provider is dynamically generated.

Now you could perform a similar lookup for each sub-object like commands, results sets, etc.. But that is expensive, you already have a connection object of the correct implementation. Hence it is smart to make the connection object a factory for any sub-objects that need to communicate with the database. That is why you can get a command object from the connection.

Answer 2

It all depends on how far you are willing to go, without breaking YAGNI principle.

Personally, I do not really like having the logic of GetStatistics method in this class and strictly speaking, I think it does break SRP.

I would rather have a separate class that reads the data, interprets it, and stores it in the Statistics class, and then inject it into DiskBook class, and then DiskBook would just be a proxy to getting its statistics. That way, if somebody was to change the format of the file where the data is read, or if the logic of calculating statistics would change, you would not have to change the DiskBook class, but the class that is responsible for parsing and calculating statistics.

Again, if this is unlikely to happen, then this is close enough and you should not overengineer the solution.