Violation & Solution for Single Responsibility Principle

Question

EDIT 2017/02/13 - Check the repo as it is now. The first example is solving with SRP in mind, but later on introducing other principles of SOLID.. I should've explained that better.

---

I'm trying to get a good example down of a violation and how to solve the SR principle. I want to be sure however it is good, and that I'm not writing down anything that doesn't make sense - so any input is welcome, or how I could better explain this. I'm doing this to teach myself and understand the "S" of SOLID better by theory and example.

Here's the example: https://github.com/learnasyougo/S.O.L.I.D#single-responsibility-principle - or below as a reference.

---

Single Responsibility Principle

A class should only have only one reason to change.

Violation example

Class Person is responsible for holding person related data, it also holds has a function Format which outputs this person data to a given format. Currently this method accepts a parameter on which the function decides which algorithm to use return the data in the required format.

The example below violates the single responsibility principle because the class Person now is responsible for keeping data of of the person and formatting that data.

class Person {
    public string FirstName { get; set; }
    public string LastName  { get; set; }
    public Gender Gender { get; set; }
    public DateTime DateOfBirth { get; set; }
    public string Format(string formatType) {
        switch(formatType) {
            case "JSON":
               // implement JSON formatting here
               return jsonFormattedString;
               break;
            case "FirstAndLastName":
              // implementation of first & lastname formatting here
              return firstAndLastNameString;
              break;
            default:
              // implementation of default formatting
              return defaultFormattedString;
        }
    }
}

How to fix?

We can solve this by making sure that the class Person is only responsible for keeping the data and not responsible for formatting that data. Thus we will extract out the method Format and introduce another class that is responsible for formatting a book.

class PersonFormatter {
    public string Format(Person person, string formatType) {
        switch(formatType) {
            case "JSON":
               // implement JSON formatting here
               return jsonFormattedString;
               break;
            case "FirstAndLastName":
              // implementation of first & lastname formatting here
              return firstAndLastNameString;
              break;
            default:
              // implementation of default formatting
              return defaultFormattedString;
        }
    }
}



class Person {
    public string FirstName { get; set; }
    public string LastName  { get; set; }
    public Gender Gender { get; set; }
    public DateTime DateOfBirth { get; set; }
}

We could argue that this example still holds violations against SOLID, and that is true. As the format method itself still holds too much responsabilities as it has to implements too much details on how it is being formatted. We'll solve this later one with oneor more of the other principles: namely the Dependency Inversion principle and the Interface Segragation Principle. But at least now we can change the implementation of the formatting of a Person without affecting the Person class itself - as the SRP states.

Answer 1

Firstly don't use a string argument to decide which type of format you want, it will make the rest look bad and it is very prone to errors the compiler cannot catch.

Secondly there is nothing wrong with an object holding data being able to format itself. The rest of the code should ideally not have to worry about which members the object has. You don't want to have to change 100 places just because you added a middle name.

Thirdly, and this is where the single responsibility comes into play, a person instance should not know how to json itself. It should only know of something it can call with the information in itself (firstname, lastname etc.) turning it all into json. It might need to create a collection of fieldnames and values and pass that to the json creator.

So create a general class that any class can use when it has to json itself that adds the brackets, delimiters etc.

And you should not have all the logic inside the general format method, split it up into a method for each type and call those from the general format method.

Answer 2

I only read as far as the word "Person", before I thought, "uh-oh". A broad entity class is bound to violate SRP eventually, in any non-trivial system.

I see so many text-books give examples of classes that attempt to model an entity, when IMO they should really be modeling a narrow range of behavior. Take POCOs for example, sure, one might be defined to represent "Person", but they really have an extremely limited range of behavior that makes sense for them. They are not defined to be able to do everything a person can do.

Thats an extreme example but the principle still apples, your classes should be defined narrowly by what they DO not broadly by what they ARE, that's how you stay on the good side of SRP.

Answer 3

class Person now is responsible for keeping data of of the person and formatting that data.

The hallmark of OOP is coupling data with behaviour. If you have a class whose whole purpose is just to hold accessors to data, what you have is an anemic domain model. An anemic class is a data structure, not an object, so you're not actually writing OOP when you separate data from behaviour. A good object hides its data and exposes only behaviors.

Although there are many reasons why a data structure plus procedural code may sometimes be an overall better solution than a real object with OOP, an anemic object fundamentally contradicts OOP.

In terms of SRP, a PersonFormatter still does too much. The PersonFormatter does formatting for JSON, for plain name, etc. What I'd suggest is to define an IPersonFormatter interface which have multiple implementations:

interface IPersonFormatter {
    public string Format(Person person);
}

class JSONPersonFormatter : IPersonFormatter {
    ...
}

class TemplateStringPersonFormatter : IPersonFormatter {
    public TemplateStringPersonFormatter(String fmtString) {
        ...
    }
    ...
}

Internationalization note: many people have a name that doesn't fit the template.