Sponsored By Aspose - File Format APIs for .NET

Aspose are the market leader of .NET APIs for file business formats – natively work with DOCX, XLSX, PPT, PDF, MSG, MPP, images formats and many more!

The 98th Thing Every Programmer Should Know

I am sitting right now in the “Opening Party” for the 97 Things Programmer Should Know. Someone had mentioned this to me when I was touring in South Africa and I had never gotten a chance to put together an entry so here it is. Thanks to Thomas Huberz for letting me quote from his book while people are talking in Norwegian.

 

Responsibility is Important When Dealing With Single Responsibility

Many people apply Single Responsibility Principle (SRP) way past where it should be. SRP is defined in an article in the book by Uncle Bob Martin as only needing a single reason to change. He gives an example as follows:

public class Employee {
    public Money CalculatePay() …
    public String ReportHours() …
    public void save() …
}

The suggestion is to separate these methods into three separate classes.

public class Employee {
   public Money CalculatePay() …
}

public class EmployeeReporter {
    public String ReportHours() …
}

public class EmployeeRepository {
    public void save() …
}

The explanation provided is

Some programmers might think that putting these three functions in the same class is perfectly appropriate. After all, classes are supposed to be collections of functions that operate on common variables. However the problem is that the three functions change for completely different reasons. The CalculatePay method will change whenever the business rules for calculating pay do. The ReportHours method will change whenever someone wants a different format for the report. The Save function will change whenever the DBAs change the database schema. These three reasons to change make Employee very volatile. It will change for any of those reasons.

Uncle Bob’s example is an extreme case of a SRP violation. Many tend to end up in less obvious situations such as the following.

public class Employee {
   public void CalculatePay() …
   public void ChangeSex() …
   public void UpgradePayGrade() …
}

By the logic presented in the explanation we could very easily argue that these three methods should also be in three separate classes. The CalculatePay method will change whenever the business rules for calculating pay do. The ChangeSex method will change whenever the business rules for changing the sex of an employee change. The UpgradePayGrade method will change whenever the logic for upgrading the grade of a user changes. Should we however separate these methods off of the Employee object?

public class EmployeePaymentCalculator {
    public void CalculatePay(Employee employee) …
}

public class EmployeeSexChanger {
    public ChangeSex(Employee employee) …
}

public class PayGradeUpgrader {
    public UpgradePayGrade(Employee employee) …
}

The basis for the suggestion of SRP is that each class should only have one reason for change. This is all fine and good so long as people temper that suggestion with what the responsibility of an object is. The responsibility of an object is to encapsulate data and provide behaviors. Without taking into account the basics of what an object’s responsibilities are one could follow SRP to the demise of their object oriented code and easily end up with procedural code and many do. I want to make it clear that Bob does specify that object responsibilities are important but many tend to miss this subtle detail.

One can find a quick smell to help identify the places where we may have incorrectly applied SRP and created procedural code by noticing the parameter arguments that are being passed. If we are passing an Employee how are we accessing it? Are we breaking state encapsulation? If we have getters and setters being called in that method we should really be reconsidering whether that code actually falls under the responsibility of our object.

In the case above we also have a tradeoff between SRP and cohesion. Cohesion is a very important concept as well as the reasons for change. If however we are responsible, and keep in mind the responsibilities of an object when applying single responsibility principle we can end up in the happy middle ground between god classes and procedural code.

This entry was posted in Uncategorized. Bookmark the permalink. Follow any comments here with the RSS feed for this post.

15 Responses to The 98th Thing Every Programmer Should Know

  1. Sebastian says:

    Nice article. I think the SRP goes hand in hand with double dispatch for not breaking encapsulation such as:

    Employee {
    String employeeToken;

    Money calculatePay(EmployeePaymentCalculator)
    {
    return EmployeePaymentCalculator.doCalculate(this.employeeToken);
    }

    }

    In Java, .Net hybrid languages we have no choice other than using MethodLess roles, so maybe a Person may wear a UnsatisfiedWithSex hat (as an interface) and somehow isolate that use case from others. to represent the user mental model more descriptive:

    class Person implements UnsatisfiedWithSex {

    changeSex(SurgeryInstitute) {
    if (SurgeryInstitute.canChangeSex(this.attributes) {
    this.sex = this.sex.reverse();
    }
    }

    }

    :)

  2. amir says:

    Why dont you do something like this

    class Employee {

    public decimal CalculatePay(decima amount) {

    return new PayCalculator.CalculatePay(amount);

    }

    }

  3. Avi Block says:

    @Jarrett
    There are other ways around that.
    You can use a ViewModel as well. CQRS makes this even simpler.

  4. John Sonmez says:

    Good post. I agree with you 100%. The key is where the data lives. If you create another class and have to give it all of the data of the original class to do it’s work, then you might reconsider creating that new class, or consider splitting the data.

  5. Jarrett says:

    In my experience, there’s a conflict between SRP and MVC. MVC wants to avoid anemic domain models. The business goes on the model. But it can easily be remedied.

    class Employee {
    /* snip */
    public decimal CalculatePay(IPayCalculator payCalulator) {
    return payCalculator.CalculatePay(this);
    }
    }

    This works just fine. It’s a simple method-level injection refactor.

  6. Roger says:

    Interesting post and something I’ve been thinking of for a long time.

    I’m one of those who have a hard time to describe what SRP _really_ means, just because the reasons you mention (“eg person object may be changed both by changing her/his name and her/his sex).

    < >

    Hmm… Aren’t we back to the original problem? To me it’s just obvious from a gut feeling perspective that db methods shouldn’t be part of the entities but… In your repository’s Save() method – aren’t you grabbing the data out of the person object (whether it’s done by an orm, reflection, props or whatever is probably another story)?

  7. Greg says:

    hjhshsj it sounds like you are trying to make an argument that procedural code is more maintainable? Having many single method objects with low cohesion is not maintainable, there are trade offs involved.

  8. EmployeeSexChanger. Hope I don’t ever need one of those!

  9. Steve Py says:

    It’s a matter of scope, how “deep” you want to define what constitutes having a single reason for change.

    I’d argue that those methods do belong in the Employee (or base class) based on the arguement that any changes to them reflect a change to the employee.

    It’s perspective and to be efficient you do need to generalize. For example, take Newtonian physics which says drop a feather and a bowling ball on earth the same distance in a vaccuum and they’ll fall at the same rate. Generally, true. Actually, false. Accelleration is determined by the combination of the masses involved. The bowling ball *does* fall faster, it’s just that the difference in mass between Bowling Ball + Earth vs. Feather + Earth is relatively insignificant.

    Generalization has to apply to some extent to anything we do otherwise we miss the forest for the tree.

  10. hjhshsj says:

    Without taking into account the basics of what an object’s responsibilities are one could follow SRP to the demise of their object oriented code and easily end up with procedural code and many do

    Code maintainability is far more important than fuzzywarm feelings of pseudo-OO.

  11. Harry says:

    I was watching the DCI talks from Oredev. Actually, I think they said your data objects should be dumb. If I understand it correctly, the behaviors of the objects should be simply modifying the data inside. The other interesting behavior (business transactions) should be bound to context and provided or injected by ‘roles’. Alright, I have to admit I don’t fully understand DCI architecture. But, I believe this is somewhat related to what we are talking about here.

  12. Greg says:

    zvolkov

    2.
    a fundamental, primary, or general law or truth from which others are derived: the principles of modern physics.
    3.
    a fundamental doctrine or tenet; a distinctive ruling opinion: the principles of the Stoics.

    Is it actually a principle?

  13. zvolkov says:

    OMG. Finally somebody noticed this. Any principle can be abused to the absurd.

  14. Josh Schwartzberg says:

    Excellent post. As someone who is constantly defending himself from others as to the exact “boundary” to the scope of the “R” in SRP, this article hits me head on. In short, I answer with “it depends”, which doesn’t satisfy their scientific requirements for adhering to such an idea… even if it’s just a principle. I wish I had a better way of describing the exact stages of an object between zero cohesion and god classes and *proving* it’s near that sweet spot of maintainability and understandability (without even taking into account other important aspects like method naming and cyclomatic complexity).

  15. nico says:

    good post… i think i haven’t really grasped yet what uncle bob means exactly with his definition of SRP. Taken litterally it can be quite extreme, as you describe.

    But taken only as a rule of thumb i think it helps me watch out and not let classes grow to big. And it’s a great feeling when i see an ‘Extract Class’ refactoring taking shape.

Leave a Reply

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <s> <strike> <strong>