3

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.

  • 1
    Immediate thought is that there's a lot going on - I'd maybe try and find an example that isn't based on a DTO with lots of public properties. Maybe something that has a public interface calling a private method that should really be a separate class? – Ant P Feb 11 '17 at 12:40
  • True, I'll try and figure out a better example then. I wanted to have an example that builds on and applies all SOLID principles.. Byt maybe that's a bit too much.. Is that example I have though bad and completely wrong in my explenation, or is it just too complex as you said - on which I agree indeed. – Yves Schelpe Feb 11 '17 at 13:22
3

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.

  • I agree, I'm just trying to build up an example that will go along an applies all SOLID principles to a bad example / violation(s)... - As you said yourself, where the SRP comes into play - I try to seperate it already... But maybe I should first keep it simple, and later on come up with an example that's bigger / complexer. I'll revise this. Thannks for the advice. – Yves Schelpe Feb 11 '17 at 13:25
  • maybe I should keep the Format method, introduce and IFormatter as parameter .. ? And later on I can build on the "Interface Segragation" and leave off the Format method and introduce IFormattable - which then holds format(IFormatter fromatter); – Yves Schelpe Feb 11 '17 at 13:31
  • 2
    The class has two reasons to change, the shape and behavior of a person and the formats it's supposed to support. I think this IS a good example of SRP. – Joppe Feb 11 '17 at 13:37
  • @Joppe - I initially thought so too, but I understand it's maybe a bit contrived and complex - however it is my intention to build on it.. I changed it now to this: github.com/learnasyougo/S.O.L.I.D#openclosed-principle - where I left in the format method, but made sure the details of the implementation are extracted.. So that later on I can introduce the notion of Interface Segragation principle as well, and add "IFormattable" for example on the Person class..? – Yves Schelpe Feb 11 '17 at 13:47
  • 2
    @Yves Schelpe ToString() is fine if assists debugging or perhaps inclusion in log file entries. Or if it is on basic value objects only if there is one obvious 'default' you could do. But typically you want to defer how you present something till the last moment, the presentation layer or view model you are projecting into. – Joppe Feb 11 '17 at 16:15
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.

  • I agree.., however.. I think that most textbooks give examples which do not correspond to real world situations.. Check the repo now - where I just wanted to start out with a "bad" example and build on SOLID from there on. I ended up with something similar if you check/read the repo now. – Yves Schelpe Feb 13 '17 at 18:15
1

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.

  • I agree with your comment, check the repo - where I went the same route.. I just wanted to start out with a "bad" example and build on SOLID from there on. I ended up with something similar if you check/read the repo now. – Yves Schelpe Feb 13 '17 at 18:13
  • 4
    "The hallmark of OOP is coupling data with behaviour". Yes but this is besides the point. The point is that Person does not have person behavior, it has presentation behavior. It would be alright if it had methods like Eat and Pee because that is what persons do. Persons do however not format representations of their data properties. THAT is the SRP violation. The resposibility is "be a person". – Martin Maat Feb 13 '17 at 19:58

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.