Learning Single Responsibility Principle with C#

Let’s start with what does Single Responsibility Principle (SRP) actually mean:

A class should have only one reason to change.

This effectively means every object (class) should have a single responsibility, if a class has more than one responsibility these responsibilities become coupled and cannot be executed independently, i.e. changes in one can affect or even break the other in a particular implementation.

A definite must read for this is the source itself (pdf chapter from "Agile Software Development, Principles, Patterns, and Practices"): The Single Responsibility Principle

Having said that, you should design your classes so they ideally only do one thing and do one thing well.

First think about what “entities” you have, in your example I can see User and Channel and the medium between them through which they communicate (“messages"). These entities have certain relationships with each other:

A user has a number of channels that he has joined
A channel has a number of users

This also leads naturally do the following list of functionalities:

A user can request to join a channel.
A user can send a message to a channel he has joined
A user can leave a channel
A channel can deny or allow a user’s request to join
A channel can kick a user
A channel can broadcast a message to all users in the channel
A channel can send a greeting message to individual users in the
channel

SRP is an important concept but should hardly stand by itself – equally important for your design is the Dependency Inversion Principle (DIP). To incorporate that into the design remember that your particular implementations of the User, Message and Channel entities should depend on an abstraction or interface rather than a particular concrete implementation. For this reason we start with designing interfaces not concrete classes:

public interface ICredentials {}

public interface IMessage
{
    //properties
    string Text {get;set;}
    DateTime TimeStamp { get; set; }
    IChannel Channel { get; set; }
}

public interface IChannel
{
    //properties
    ReadOnlyCollection<IUser> Users {get;}
    ReadOnlyCollection<IMessage> MessageHistory { get; }

    //abilities
    bool Add(IUser user);
    void Remove(IUser user);
    void BroadcastMessage(IMessage message);
    void UnicastMessage(IMessage message);
}

public interface IUser
{
    string Name {get;}
    ICredentials Credentials { get; }
    bool Add(IChannel channel);
    void Remove(IChannel channel);
    void ReceiveMessage(IMessage message);
    void SendMessage(IMessage message);
}

What this list doesn’t tell us is for what reason these functionalities are executed. We are better off putting the responsibility of “why” (user management and control) in a separate entity – this way the User and Channel entities do not have to change should the “why” change. We can leverage the strategy pattern and DI here and can have any concrete implementation of IChannel depend on a IUserControl entity that gives us the "why".

public interface IUserControl
{
    bool ShouldUserBeKicked(IUser user, IChannel channel);
    bool MayUserJoin(IUser user, IChannel channel);
}

public class Channel : IChannel
{
    private IUserControl _userControl;
    public Channel(IUserControl userControl) 
    {
        _userControl = userControl;
    }

    public bool Add(IUser user)
    {
        if (!_userControl.MayUserJoin(user, this))
            return false;
        //..
    }
    //..
}

You see that in the above design SRP is not even close to perfect, i.e. an IChannel is still dependent on the abstractions IUser and IMessage.

In the end one should strive for a flexible, loosely coupled design but there are always tradeoffs to be made and grey areas also depending on where you expect your application to change.

SRP taken to the extreme in my opinion leads to very flexible but also fragmented and complex code that might not be as readily understandable as simpler but somewhat more tightly coupled code.

In fact if two responsibilities are always expected to change at the same time you arguably should not separate them into different classes as this would lead, to quote Martin, to a "smell of Needless Complexity". The same is the case for responsibilities that never change - the behavior is invariant, and there is no need to split it.

The main idea here is that you should make a judgment call where you see responsibilities/behavior possibly change independently in the future, which behavior is co-dependent on each other and will always change at the same time ("tied at the hip") and which behavior will never change in the first place.

Re-factorize a program using single responsibility principle - SOLID- SRP

You separated the responsibility of saving data (WalkingDataRepository) and also encapsulated it within WalkingDataManager. The refactoring work totally aligns with what SRP says. In short, the SRP is not just about breaking down the functionalities but also about encapsulating them! I also wrote an article about it.

What bothers me is the anemic WalkingData model. This is an anti-pattern. You should rather let WalkingData handle the responsibility of maintaining its own data rather than creating another class WalkingDataManager to do that job.

Single Responsibility Principle and Many methods in Repository

question: what are the responsibilities of this repository class?

answer : to execute operations against a database (CRUD)

further reading: http://pragmaticcraftsman.com/2006/07/single-responsibility-principle

Clarification of single responsibility principle

Let's suppose that you have a method called "HandleError" that receives an error and creates a logfile if it doesn't exist. Once it is created, it stores some needed information about the error in this log file.

For me, if you create this method in a separate class, responsible for handling the error logs and call it from your catch, you won't violate the SRP.

But if you create this method in the same class, you probably will be violating the pattern because your class will have more than one reason to change.

How single responsibility principle avoid code smell in this scenario?

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.

It this an example of the Single Responsibility Principle?

I like to think of the Single Responsibility Principle as an implementation of separation of duties. Before I start splitting my classes as you have, I try to think of what each class should be responsible for.

Your classes are quite simple and lend themselves well to an abstract class with an implemented Print() and Save() functions as you mentioned. I would tend to keep that design over your current one.

However, if printing and saving were more complicated tasks which might be performed in different ways then a dedicated Printer or Saver class would be warranted, since that responsibility is now more complex. The 'complexity' threshold for making a new class is very subjective and will depend on the exact situation, but in the end, the code is just an abstraction for us lowly humans to understand, so make it such that it's the most intuitive.

You Container class is a little misleading. It doesn't actually 'contain' anything. It actually implements the Factory Method Pattern and would benefit from being named a factory.

Also, your PersonDisplayer is never instantiated and can provide all of its functionality through static methods, so why not make it a static class? It's not uncommon for utility classes such as Printers or savers to be static. Unless you have a need to have separate instances of a printer with different properties, keep it static.

Understand Single Responsibility Principle of SOLID design

What is the purpose of ICheckValid interface? Do you call check_valid elsewhere? In my opinion, since the frame_number seems to be in fact a read-only property of a Frame, why it would be wrong to verify its consistency right in the constructor without any additional interfaces for that? (Constructors are supposed to produce consistent objects and are thus free to validate incoming parameters however they like.)

However, rather than to ask how to properly validate this field, it might be better to ask why indeed you need the frame_number property in the Frame? It seems like this is an index of this item in some array - you may just use the index, why store it in the Frame? You may want to write some if/else logic later, such as:

if (frame_number == 10) {
     // some rules
} else {
     // other rules
}

However, this is unlikely a SOLID approach as you would probably end up writing this if/else statements in many parts of the Frame. Rather, you may create a base class FrameBase, define much of the logics there plus some abstract methods to be implemented in OrdinaryFrame and TenthFrame, where you would define different rules. This would enable you to avoid frame_number altogether -- you would just create nine OrdinaryFrames and one TenthFrame.

As for critique: your code seems to abstract balls and frames, but ignores 'throws', or 'rolls' for some reason. Consider a need to add trajectory information of each roll, you would need to change the IFrame interface, adding something like void SetThrowTrajectory(int throwNumber, IThrowTrajectory trajectory). However, if you abstract throws away in an e.g. IBallRoll, the trajectory-related functionality would easily fit there (as well as some Boolean computed properties, e.g. IsStrike, IsSpare).

Understanding the practical benefits of using the Single Responsibility Principle

To me the modem example above always seemed like a case for the interface segregation principle rather than SRP, but that's besides the point.

In the part you called out regarding the Rectangle, I think you're just misinterpreting it. Martin is using the Rectangle as an example of a shared library. If the GraphicalApplication requires a new method or change in semantics in the Rectangle class, then that affects the ComputationalGeometryApplication since they both "link" to the Rectangle library. He's saying it violates SRP because it is responsible for defining rendering bounds and also an algebraic concept. Imagine if the GraphicalApplication changed from DirectX to OpenGL where the y-coordinate is inverted. You may want to change some things around on the Rectangle to facilitate this, but you're then potentially causing changes in ComputationalGeometryApplication.

In my work, I try to follow the SOLID principles and TDD, and I've found that SRP makes writing tests for classes simple and also keeps the classes focused. Classes that follow SRP are typically very small, and this reduces complexity both in code and dependencies. When stubbing out classes, I try to make sure a class is either "doing one thing", or "coordinating two (or more) things". This keeps them focused and makes their reasons for change only dependent on the one thing they do, which to me is the point of SRP.

Using the Single Responsibility Principle forces my containers to have public setters

Most data access strategies (ORM techniques) involve compromises of the kind you're describing. The least intrusive ones use reflection (or introspection, etc.) to populate your entities when they need to (when no public setters exist, for example).

Having said that, if your entities are only data containers (as in the anemic domain model), the single responsibility principle shouldn't really concern you much, since your objects don't have complex logic that would be spoiled by (or interfere with) data access code.

Learning Single Responsibility Principle with C#