63

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.

  • 19
    this class violates SRP not because it mixes data with logic, but because it has low cohesion - potential God object – gnat Jan 7 '16 at 21:39
  • 1
    What purpose is adding holidays to the employee? There should be perhaps a calendar class or something that has the holidays. I think your friend is right. – James Black Jan 8 '16 at 1:48
  • 9
    Never listen to somebody who says "I am a believer in X". – Stig Hemmer Jan 8 '16 at 8:47
  • 29
    It's not so much whether this is a violation of the SRP as whether it is good modeling at all. Suppose I am an employee. When I ask my manager if it's OK with him if I take a long weekend to go skiing, my manager does not add holidays to me. The code here does not match the reality it intends to model, and therefore is suspicious. – Eric Lippert Jan 8 '16 at 15:31
  • 2
    +1 for functional programming being the only way to heaven. – erip Jan 9 '16 at 21:48

11 Answers 11

68

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.

  • If Object Oriented Programming was created and is intended to model the real life objects and classes (actions + attributes) then why not write the code class having multiple responsibilities (actions)? A real world object can have multiple responsibilities. For example a journalist writes editorials in news papers and interviews politicians in a TV show. Two responsibilities for a real life object! What if I am going to write a class Journalist? – user1451111 Sep 8 '18 at 13:43
41

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.

20

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.

  • I agree. If the code is as simple as provided here, I would probably let it slide. But in my mind, I should not be the responsibility of the Employee to handle its own holiday. It might not seem like a big deal with where the responsibility is placed, but look at it this way: If you were new to the code base, and had to work on the holiday-specific functionality - where would you look first? For the holiday-logic, I personally would NOT look at the Employee entity to begin with. – Niklas H Jan 8 '16 at 13:37
  • 1
    @NiklasH Agreed. Tho personally I wouldn't look randomly and try and guess the class I would search with studio for the word "Holiday" and see what classes it came up in. :) – NikolaiDante Jan 8 '16 at 14:21
  • 4
    True. But what if it is not called "Holiday" in this new system, but is "Vacation" or "Free Time". But I agree, with that you typically has the ability to just search for it, or can ask a co-worker. My comment were primarily to get the OP to mentally model the responsibility and where the most obvious place for the logic would be :-) – Niklas H Jan 8 '16 at 14:30
  • I agree with your first statement. However, if it came to peer review, I don't think I would because violating SRP is a slippery slope and this could be the first of many broken windows. Cheers. – Jim Speaker Jun 13 '17 at 19:31
20

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.

9

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.

  • Interesting line of thinking, thanks a lot for sharing – tobiak777 Jan 8 '16 at 13:55
  • Nice - Good way to express the intended goals of OOP. – user949300 Jan 9 '16 at 21:56
5

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.

  • "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." + Employee might have several name etc... The point of a model is to focus on the relevant facts of the real world for the problem at hand. There exists requirements for which this model is optimal. In these requirements, Employees are only interesting to the extent that we can modify their holidays and we're not interested too much in managing other aspects of their real life specifics. – tobiak777 Jan 14 '16 at 0:46
  • @reddy "Employees are only interesting to the extent that we can modify their holidays" - Which means you need to identify them correctly. As soon as you have an employee, they can change their last name at any time due to marriage or divorce. They can also change their name and gender. Will you fire employees if their lastname changes such that their name matches that of another employee? You will not add all of this functionality right now. Instead you'll add it when you need it, which is good. Irrespective of how much is implemented, the responsibility of identification stays the same. – Peter Jan 14 '16 at 11:52
3

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

  • No! SRP is not violated by multiple pieces of logic, multiple pieces of data or any combination of the two. The only requirement is the object should stick to its purpose. Its purpose can potentially entail many operations. – Martin Maat Dec 18 '17 at 20:23
  • @MartinMaat: Many operations, yes. One piece of logic as a result. I think we're saying the same thing but with different terms (and I am happy to assume that yours are the correct ones as I don't study this stuff often) – Lightness Races with Monica Dec 21 '17 at 11:19
2

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.

1

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.

  • Thanks for your input Vince. The point is, you don't need a service layer when you have a rich domain. There's only one class responsible for the behaviour, and it is your domain entity. The other layers (web layer, UI etc...) typically deal with DTOs, and ViewModels and this is fine. The domain model is to model the domain, not to do UI works, or send messages across the internet. Your message reflects this common misconception, which comes from the fact that people simply do not know how to fit OOP into their design. And I think this is very sad - for them. – tobiak777 Jan 14 '16 at 0:26
  • I don't think I have a misconception of rich domain OOP, I've designed a lot of sotwares that way, and it is true it is really good for maintenance / evolution. But I'm sorry to tell you this is not a silver bullet. – Vince Jan 14 '16 at 10:07
  • I'm not saying it is. For writing a compiler it probably isn't, but for most of the line of business / SaaS applications, I think it is much less art/science than what you suggest. The need for domain model can be proven mathematically, your examples make me think of debatable designs rather than flaws of limitations in OOP – tobiak777 Mar 24 '16 at 12:01
0

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.

0

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.

  • 1
    This is hardly a real answer, but it would have been great as a comment to the question. – Jay Elston Jan 26 '16 at 2:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.