How single responsibility principle avoid code smell in this scenario?

Question

I'm new to Design Patterns, I know the purpose of single responsibility principle, but not 100% sure how it can avoid lots of tiny changes. Below is my example:

//very crude implementation
public class Journal
{
    private readonly List<string> entries = new List<string>();
    private static int count = 0;

    public void AddEntry(string text)
    {
       entries.Add($"{++count}: {text}");
    }

    public void RemoveEntry(int index)
    {
       entries.RemoveAt(index);
       count--;
    }

    public void SaveToDisk(string filename)
    {
       File.WriteAllText(filename, ToString());
    }
}

I know the SaveToDisk method should not be included in the class, it should be a dedicated class, like PersistenceManager to handle the file saving.

But why can't I keep the SaveToDisk() method in Journal class? if there is any new requirements such as Save the Journal to cloud, then I just add a new method SaveToCloud(), and any dependent client classes can use SaveToCloud(), the only modification I need to make is adding SaveToCloud() in Journal class, which is totally fine?

Edited: below is my modified version, please spot any design errors:

class Program
{
  static void Main(string[] args)
  {
    Consumer client = new Consumer(new DiskManager("C:\\journal.txt"));
    // consumer add text to Journal
    client.AddJournal("sometext");
    client.SaveJournal();
  }
}

public class Journal
{
  private readonly List<string> entries = new List<string>();

  public void AddEntry(string text)
  {
    entries.Add(text);
  }

  public void RemoveEntry(int index)
  {
    entries.RemoveAt(index);
  }
}

public interface IPersistenceManager
{
  void Save(Journal journal);
}

public class DiskManager : IPersistenceManager
{
  private string filePath;

  public DiskManager(string filePath)
  {
    this.filePath = filePath;
  }

  public void Save(Journal journal)
  {
    //XXX.XXX.Save(filePath);
  }
}

public class CloudManager : IPersistenceManager
{
  private string url;

  public CloudManager(string url)
  {
    this.url = url;
  }

  public void Save(Journal journal)
  {
    //XXX.XXX.Save(url);
  }
}


public class Consumer
{
  private Journal _journal = new Journal();
  private IPersistenceManager _manager;

  public void AddJournal(string note)
  {
    _journal.AddEntry(note);
  }

  public Consumer(IPersistenceManager manager)
  {
    _manager = manager;
  }
  public void SaveJournal()
  {
    _manager.Save(_journal);
  }
}

Answer 1

Encapsulation

By moving the code for persistence into a separate PersistenceManager class, you guarantee that the SaveToDisk() method will not modify any of the private variables of a journal, except by using the public methods and properties of the journal.

Single Responsibility

But why can't I keep the SaveToDisk() method in Journal class? If there is any new requirements such as Save the Journal to cloud, then I just add a new method SaveToCloud(), and any dependent client classes can use SaveToCloud(), the only modification I need to make is adding SaveToCloud() in Journal class, which is totally fine?

Saving the journal to the cloud will require you to maintain some extra state - a connection string, an api key, maybe a blob client, etc. Where do you get that state?

• You could store it all as static members within the Journal class
• You could pass it all in to the SaveToCloud() method as parameters

Storing it as static members is rather limiting, and you can run in to concurrency issues.

Passing the parameters in to the SaveToCloud() method every time means you need to go through every class that originally called SaveToDisk(), and update it to store the parameters you need. These are the 'lots of tiny changes' that you want to avoid.

If instead, you made a PersistenceManager class, with a Save() method, then you can add the connection string etc to this class, and none of the callers need to change.

Dependency Inversion

Entities must depend on abstractions not on concretions.

By implementing this as a static method in the Journal class, you remove the possibility for dependency inversion. Classes that want to save journals should define this as an interface:

public interface IPersistenceManager
{
    void Save(string name);
}

Notice it doesn't say ToDisk() on the end - the callers shouldn't care where the journal is being saved, as long as it is being saved. Then when your requirements change from storing on disk to storing in the cloud, you don't need to make any code changes to the callers.

Answer 2

The Journal class' job -- its single responsibility -- is to represent journals. The problem with a method like Journal.saveToDisk() it that it requires work that is not part of that job, like deciding what filename to use, making sure nothing is left in a bad state if the process aborts, etc.. The Journal class shouldn't have to know anything about how to deal with disks.

As you recognized, this is the thin edge of a bad wedge. Soon enough you will need a Journal.saveToCloud() and the Journal class will have to know all about networking, too.

In a situation like this, you have to separate the journal work from the disk work, so you can put the journal work in the Journal class and the disk work elsewhere.

A common solution would be to add byte [] Journal.toByteArray() that converts a journal to data that you could then save to either the disk, or the cloud, or anywhere else. Journal.writeToStream(Stream target) would perform a similar function. Either way, translating a journal into a sequence of bytes is journal-specific work that you can put in the Journal class.

Whether or not you need anything like a persistence manager depends on the rest of your application.