Does this class design violate the single responsibility principle?

Question

Today I had an argument with someone.

I was explaining the benefits of having a rich domain model as opposed to an anemic domain model. And I demoed my point with a simple class looking like that:

public class Employee
{
    public Employee(string firstName, string lastName)
    {
        FirstName = firstName;
        LastName = lastname;
    }

    public string FirstName { get private set; }
    public string LastName { get; private set;}
    public int CountPaidDaysOffGranted { get; private set;}

    public void AddPaidDaysOffGranted(int numberOfdays)
    {
        // Do stuff
    }
}

As he defended his anemic model approach, one of his arguments was: "I am a believer in SOLID. You are violating the single responsibility principle (SRP) as you are both representing data and performing logic in the same class."

I found this claim really surprising, as following this reasoning, any class having one property and one method violates the SRP, and therefore OOP in general is not SOLID, and functional programming is the only way to heaven.

I decided not to reply to his many arguments, but I am curious what the community thinks on this question.

If I had replied, I would have started by pointing to the paradox mentioned above, and then indicate that the SRP is highly dependent on the level of granularity you want to consider and that if you take it far enough, any class containing more than one property or one method violates it.

What would you have said?

Update: The example has been generously updated by guntbert to make the method more realistic and help us focus on the underlying discussion.

Answer 1

Single Responsibility should be understood as an abstraction of logical tasks in your system. A class should have the single responsibility to (do everything necessary in order to) perform one single, specific task. This can actually bring a lot into a well-designed class, depending on what the responsibility is. The class that runs your script engine, for example, can have a lot of methods and data involved in processing scripts.

Your coworker is focusing on the wrong thing. The question is not "what members does this class have?" but "what useful operation(s) does this class perform within the program?" Once that is understood, your domain model looks just fine.

Answer 2

Single Responsibility Principle is only concerned with whether or not a piece of code (in OOP, typically we're talking about classes) has responsibility over one piece of functionality. I think your friend saying that functions and data can't co-mingle didn't really understand that idea. If Employee were to also contain information about his workplace, how fast his car goes, and what type of food his dog eats then we'd have a problem.

Since this class deals only with an Employee, I think it's fair to say it doesn't violate SRP blatantly, but people are always going to have their own opinions.

One place where we might improve is to separate Employee information (like name, phone number, email) from his vacation. In my mind, this doesn't mean that methods and data cannot co-mingle, it just means that perhaps the vacation functionality could be in a separate place.

Answer 3

There are already great answers pointing out that SRP is an abstract concept about logical functionality, but there are subtler points that I think are worth adding.

The first two letters in SOLID, SRP and OCP, are both about how your code changes in response to a change in requirements. My favorite definition of the SRP is: "a module/class/function should have only one reason to change." Arguing about likely reasons for your code to change is much more productive than arguing about whether your code is SOLID or not.

How many reasons does your Employee class have to change? I don't know, because I don't know the context in which you're using it, and I also can't see the future. What I can do is brainstorm possible changes based on what I've seen in the past, and you can subjectively rate how likely they are. If more than one scores between "reasonably likely" and "my code has already changed for that exact reason", then you are violating SRP against that kind of change. Here's one: someone with more than two names joins your company (or an architect reads this excellent W3C article). Here's another: your company changes how it allocates holiday days.

Notice that these reasons are equally valid even if you remove the AddHolidays method. Plenty of anemic domain models violate the SRP. Many of them are just database-tables-in-code, and it's very common for database tables to have 20+ reasons to change.

Here's something to chew on: would your Employee class change if your system needed to track employee salaries? Addresses? Emergency contact info? If you said "yes" (and "likely to happen") to two of those, then your class would be violating SRP even if had no code in it yet! SOLID is about processes and architecture as much as it's about code.

Answer 4

To my mind this class could potentially violate SRP if it continued to represent both an Employee and EmployeeHolidays.

As it is, and if it came to me for Peer Review, I'd probably let it through. If more Employee specific properties and methods were added, and more holiday specific properties were added I would probably advise a split, citing both SRP and ISP.

Answer 5

That the class represents data is not the responsibility of the class, it is a private implementation detail.

The class has one responsibility, to represent an employee. In this context, that means it presents some public API that provides you with functionality you need to deal with employees (whether AddHolidays is a good example is debatable).

The implementation is internal; it so happens that this needs some private variables and some logic. That doesn't mean that the class now has multiple responsibilities.

Answer 6

The idea that mixing logic and data in any way is always wrong, is so ridiculous it doesn't even merit discussion. However, there is indeed a clear violation of single responsibility in the example, but it's not because there's a property DaysOfHolidays and a function AddHolidays(int).

It's because the employee's identity is intermingled with the holiday management, which is bad. The employee's identity is a complex thing that's required to track vacation, salary, overtime, to represent who's on which team, to link to performance reports, etc. An employee can also change first name, last name, or both, and stay the same employee. Employees may even have multiple spellings of their name such as an ASCII and a unicode spelling. People can have 0 to n first and/or last names. They may have different names in different jurisdictions. Tracking the identity of an employee is enough of a responsibility that holiday or vacation management cannot be added on top without calling it a second responsibility.

Answer 7

"I am a believer in SOLID. You are violating the single responsibility principle (SRP) as you are both representing data and performing logic in the same class."

Like the others, I disagree with this.

I would say that the SRP is violated if you are performing more than one piece of logic in the class. How much data need be stored within the class to achieve that single piece of logic is irrelevant.

Answer 8

I don't find it useful these days to debate about what does and doesn't constitute a single responsibility or a single reason to change. I would propose a Minimum Grief Principle in its place:

Minimum Grief Principle: code should either seek to minimize its probability of requiring changes or maximize the ease of being changed.

How's that? Shouldn't take a rocket scientist to figure out why this can help reduce maintenance costs and hopefully it shouldn't be a point of endless debate, but as with SOLID in general, it's not something to apply blindly everywhere. It's something to consider while balancing trade-offs.

As for the probability of requiring changes, that goes down with:

1. Good testing (improved reliability).
2. Involving only the bare minimum code required to do something specific (this can include reducing afferent couplings).
3. Just making the code badass at what it does (see Make Badass Principle).

As for the difficulty of making changes, it goes up with efferent couplings. Testing introduces efferent couplings but it improves reliability. Done well, it generally does more good than harm and is totally acceptable and promoted by the Minimum Grief Principle.

Make Badass Principle: classes that are used in many places should be awesome. They should be reliable, efficient if that ties to their quality, etc.

And the Make Badass Principle is tied to Minimum Grief Principle, since badass things will find a lower probability of requiring changes than things which suck at what they do.

I would have started by pointing to the paradox mentioned above, and then indicate that the SRP is highly dependent on the level of granularity you want to consider and that if you take it far enough, any class containing more than one property or one method violates it.

From an SRP standpoint a class which barely does anything would certainly have only one (sometimes zero) reasons to change:

class Float
{
public:
    explicit Float(float val);
    float get() const;
    void set(float new_val);
};

That practically has no reasons to change! It's better than SRP. It's ZRP!

Except I would suggest it is in blatant violation of the Make Badass Principle. It's also absolutely worthless. Something which does so little cannot hope to be badass. It has too little information (TLI). And naturally when you have something which is TLI, it cannot do anything really meaningful, not even with the information it encapsulates, so it has to leak it to the outside world in hopes that someone else will actually do something meaningful and badass. And that leakiness is okay for something which is just meant to aggregate data and nothing more, but that threshold is the difference as I see between "data" and "objects".

Of course something which is TMI is bad as well. We might put our entire software in one class. It can even just have one run method. And someone might even argue that it now has one very broad reason to change: "This class will only need to be changed if the software needs improvement." I'm being silly, but of course we can imagine all the maintenance issues with that.

So there's a balancing act as to the granularity or coarseness of the objects you design. I often judge it by how much information you have to leak to the outside world, and how much meaningful functionality it can perform. I often find the Make Badass Principle helpful there to find the balance while combining it with the Minimum Grief Principle.

Answer 9

On the contrary, for me Anemic domain model breaks some OOP main concepts (which tie together attributes and behavior), but can be inevitable based on architectural choices. Anemic domains are easier to think, less organic and more sequential.

Many systems tend to do this when multiple layers must play with the same data (service layer, web layer, client layer, agents...).

It is easier to define the data structure in one place and the behavior in other service classes. If the same class was used on multiple layers, this one may grow big, and it asks the question of which layer is responsible to define the behavior it needs, and who is able to call the methods.

For example, it may not be a good idea than an Agent process which computes statistics on all your employees can call a compute for paid days. And the employee list GUI certainly not need at all the new aggregated id computation used in this statistic agent (and the technical data coming with it). When you segregate the methods this way, you generally end with a class with only data structures.

You can too easily serialize / deserialize the "object" data, or just some of them, or to another format (json)... without worrying about any object concept / responsibility. It is just data passing though. You can always do data mapping between two classes (Employee, EmployeeVO, EmployeeStatistic…) but what does Employee really mean here?

So yes it separates completely data in domain classes and data handling in service classes but it is here needed. Such system is at the same time a functional one to bring business value and a technical one too to propagate the data everywhere it is needed while keeping a proper responsibility scope (and distributed object does not solve this either).

If you don’t need to separate behavior scopes you are more free to put the methods in service classes or in domain classes, depending on how you see your object. I tend to see an object as the "real" concept, this naturally help to keep SRP. So in you example, it is more realist than the Employee's Boss add payday off granted to his PayDayAccount. An Employee is Hired by company, Works, can be Sick or be Asked for an advice, and he has a Payday account (the Boss may retrieve it directly from him or from a PayDayAccount registry...) But you can make an aggregated shortcut here for simplicity if you don't want too much complexity for a simple software.

Answer 10

You are violating the single responsibility principle (SRP) as you are both representing data and performing logic in the same class.

It sounds very reasonable for me. Model might not have public properties if exposes actions. It is basically a Command-Query Separation idea. Please note that Command will have private state for sure.

Answer 11

You cannot violate the Single Responsibility Principle because it is just an esthetic criterion, not a rule of Nature. Don't be misled by the scientifically sounding name and the uppercase letters.

Answer 12

You CANNOT properly address this question in isolation of the rest of the solution. Whether something meets or does not meet the SRP depends upon the entire solution.

In his clean coder blog Uncle Bob said:

Another wording for the Single Responsibility Principle is: Gather together the things that change for the same reasons. Separate those things that change for different reasons. If you think about this you’ll realize that this is just another way to define cohesion and coupling. We want to increase the cohesion between things that change for the same reasons, and we want to decrease the coupling between those things that change for different reasons.

If there is just one other module that interacts with Employee and it cares about all the factors present in Employee in this implementation then YES you are meeting the SRP.

This is because in terms of your solution this code IS cohesive. All the necessary components for its purpose and ONLY its purpose are contained together.

If however, you have mixed concerns in your class between different high level objectives then it should be broken up into separate classes.