Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

Simply what are the practical techniques people use to check if a class violates the single responsibility principle?

I know that a class should have only one reason to change, but that sentence is somewhat lacking a practical way to really implement that.

The only way I found is to use the sentence "The ......... should ......... itself." where the first space is the class name and the later is the method (responsibility) name.

However, sometimes it's hard to figure out if a responsibility really violates the SRP.

Are there more ways to check for the SRP?

Note:

The question is not about what the SRP means, but rather a practical methodology or a series of steps to check and implement the SRP.

UPDATE

Report class

I have added a sample class that clearly violates the SRP. It would be great if people could use it as an example to explain how they approach the single responsibility principle.

The example is from here.

share|improve this question
    
This is an interesting rule, but you still could write: "A Person Class Can Render Itself". This may be considered a violation for SRP, since including the GUI in the same class that contains business rules and data persistence is not OK. So I think you need to add the concept of architectural domains (tiers and layers) and make sure that this statement is valid with 1 of those domain only (such as GUI, Data Access, etc.) –  Emmad Kareem Jul 31 '12 at 16:23
    
@EmmadKareem This rule was mentioned in Head First Object-Oriented Analysis and Design and that's exactly what I thought about this it. It's somewhat lacking a practical way to implement it. They mentioned that sometimes the responsibilities won't be that apparent to the designer and he must used a great deal of common sense to judge if the method should really be in this class or not. –  Songo Jul 31 '12 at 16:43
    
If you really want to understand SRP, read some of Uncle Bob Martin's writings. His code is some of the prettiest I've seen, and I trust that whatever he says about SRP is not only sound advice, but is also more than just hand-waving. –  Robert Harvey Jul 31 '12 at 20:42
    
And would the down-voter please explain why to improve the post?! –  Songo Aug 1 '12 at 2:48
    
Thank you Songo for following up with an example. –  Emmad Kareem Aug 6 '12 at 10:47
add comment

6 Answers

up vote 4 down vote accepted

The SRP states, in no uncertain terms, that a class should only ever have one reason to change.

Deconstructing the "report" class in the question, it has three methods:

  • printReport
  • getReportData
  • formatReport

Ignoring the redundant Report being used in every method, it is easy to see why this violates the SRP:

  • The term "print" implies some kind of UI, or an actual printer. This class therefore contains some amount of UI or presentation logic. A change to the UI requirements will necessitate a change to the Report class.

  • The term "data" implies a data structure of some kind, but doesn't really specify what (XML? JSON? CSV?). Regardless, if the "contents" of the report ever change, then so will this method. There is coupling to either a database or a domain.

  • formatReport is just a terrible name for a method in general, but I'd assume by looking at it that it once again has something to do with the UI, and probably a different aspect of the UI than printReport. So, another, unrelated reason to change.

So this one class is possibly coupled with a database, a screen/printer device, and some internal formatting logic for logs or file output or whatnot. By having all three functions in one class, you're multiplying the number of dependencies and tripling the probability that any dependency or requirement change will break this class (or something else that depends on it).

Part of the problem here is that you've picked a particularly thorny example. You should probably not have a class called Report, even if it only does one thing, because... what report? Aren't all "reports" completely different beasts, based on different data and different requirements? And isn't a report something that's already been formatted, either for screen or for print?

But, looking past that, and making up a hypothetical concrete name - let's call it IncomeStatement (one very common report) - a proper "SRPed" architecture would have three types:

  • IncomeStatement - the domain and/or model class that contains and/or computes the information that appears on formatted reports.

  • IncomeStatementPrinter, which would probably implement some standard interface like IPrintable<T>. Has one key method, Print(IncomeStatement), and maybe some other methods or properties for configuring print-specific settings.

  • IncomeStatementRenderer, which handles screen rendering and is very similar to the printer class.

  • You could also eventually add more feature-specific classes like IncomeStatementExporter / IExportable<TReport, TFormat>.

This is made significantly easier in modern languages with the introduction of generics and IoC containers. Most of your application code does not need to rely on the specific IncomeStatementPrinter class, it can use IPrintable<T> and thus operate on any kind of printable report, which gives you all of the perceived benefits of a Report base class with a print method and none of the usual SRP violations. The actual implementation need only be declared once, in the IoC container registration.

Some people, when confronted with the above design, respond with something like: "but this looks like procedural code, and the whole point of OOP was to get us -away- from the separation of data and behavior!" To which I say: wrong.

The IncomeStatement is not just "data", and the aforementioned mistake is what causes a lot of OOP folks to feel they are doing something wrong by creating such a "transparent" class and subsequently start jamming all kinds of unrelated functionality into the IncomeStatement (well, that and general laziness). This class may start out as just data but, over time, guaranteed, it will end up as more of a model.

For example, a real income statement has total revenues, total expenses, and net income lines. A properly-designed financial system will most likely not store these because they are not transactional data - in fact, they change based on the addition of new transactional data. However, the calculation of these lines is always going to be exactly the same, no matter whether you are printing, rendering, or exporting the report. So your IncomeStatement class is going to have a fair amount of behaviour to it in the form of getTotalRevenues(), getTotalExpenses(), and getNetIncome() methods, and probably several others. It is a genuine OOP-style object with its own behaviour, even if it doesn't really seem to "do" much.

But the format and print methods, they have nothing to do with the information itself. In fact, it's not too unlikely that you'll want to have several implementations of these methods, e.g. a detailed statement for management and a not-so-detailed statement for the shareholders. Separating these independent functions out into different classes gives you the ability to choose different implementations at runtime without the burden of a one-size-fits all print(bool includeDetails, bool includeSubtotals, bool includeTotals, int columnWidth, CompanyLetterhead letterhead, ...) method. Yuck!

Hopefully you can see where the above, massively-parameterized method goes wrong, and where the separate implementations go right; in the single-object case, every time you add a new wrinkle to the printing logic, you have to change your domain model (Tim in finance wants page numbers, but only on the internal report, can you add that?) as opposed to just adding a configuration property to one or two satellite classes instead.

Implementing the SRP properly is about managing dependencies. In a nutshell, if a class already does something useful, and you are considering adding another method that would introduce a new dependency (such as a UI, a printer, a network, a file, whatever), don't. Think about how you could add this functionality in a new class instead, and how you could make this new class fit into your overall architecture (it's pretty easy when you design around dependency injection). That is the general principle/process.


Side note: Like Robert, I patently reject the notion that an SRP-compliant class should have only one or two state variables. Such a thin wrapper could rarely be expected to do anything truly useful. So don't go overboard with this.

share|improve this answer
    
+1 great answer indeed. However, I'm just confused about the class IncomeStatement. Does your proposed design mean that the IncomeStatement will have instances of IncomeStatementPrinter & IncomeStatementRenderer so that when I call print() on IncomeStatement it will delegate the call to IncomeStatementPrinter instead? –  Songo Aug 7 '12 at 10:09
    
@Songo: Absolutely not! You should not have cyclic dependencies if you're following SOLID. Apparently my answer didn't make it clear enough that the IncomeStatement class doesn't have a print method, or a format method, or any other method that doesn't directly deal with inspecting or manipulating the report data itself. That's what those other classes are for. If you want to print one, then you take on a dependency to the IPrintable<IncomeStatement> interface which is registered in the container. –  Aaronaught Aug 8 '12 at 1:05
    
aah I see your point. However, where is the cyclic dependency if I inject a Printer instance in the IncomeStatement class? the way I imagine it is when I call IncomeStatement.print() it will delegate it to IncomeStatementPrinter.print(this, format). What is wrong with this approach?...Another question, You mentioned that IncomeStatement should contain the information that appears on formatted reports if I want it to be read from the database or from an XML file, should I extract the method that loads the data into a separate class and delegate the call to it in IncomeStatement? –  Songo Aug 8 '12 at 3:08
    
@Songo: You have IncomeStatementPrinter depending on IncomeStatement and IncomeStatement depending on IncomeStatementPrinter. That's a cyclic dependency. And it's just bad design; there's no reason at all for the IncomeStatement to know anything about a Printer or IncomeStatementPrinter - it's a domain model, it's not concerned with printing, and the delegation is pointless since any other class can create or acquire an IncomeStatementPrinter. There's no good reason to have any notion of printing in the domain model. –  Aaronaught Aug 9 '12 at 3:20
    
As for how you load the IncomeStatement from the database (or XML file) - typically, that's handled by a repository and/or mapper, not the domain, and once again, you don't delegate to this in the domain; if some other class needs to read one of these models then it asks for that repository explicitly. Unless you're implementing the Active Record pattern I guess, but I'm really not a fan. –  Aaronaught Aug 9 '12 at 3:22
show 2 more comments

The way I check for the SRP is to check every method (responsibility) of a class and ask the following question:

"Will I ever need to change the way I implement this function?"

If I find a function that I'll need to implement in different ways (depending on some kind of configuration or condition) then I know for sure that I need an extra class to handle this responsibility.

share|improve this answer
add comment

Here's a quote from rule 8 of Object Calisthenics:

Most classes should simply be responsible for handling a single state variable, but there are a few that will require two. Adding a new instance variable to a class immediately decreases the cohesion of that class. In general, while programming under these rules, you’ll find that there are two kinds of classes, those that maintain the state of a single instance variable, and those that coordinate two separate variables. In general, don’t mix the two kinds of responsibilities

Given this (somewhat idealistc) view, you could say that any class which contains only one or two state variables is unlikely to violate SRP. You could also say that any class which contains more than two state variables may violate SRP.

share|improve this answer
2  
This view is hopelessly simplistic. Even Einstein's famous, but simple equation requires two variables. –  Robert Harvey Jul 31 '12 at 21:11
    
The OPs question was "Are there more ways to check for the SRP?" - this is one possible indicator. Yes it's simplistic, and it doesn't hold up in every case, but it is one possible way to check that SRP has been violated. –  MattDavey Aug 1 '12 at 7:40
1  
I suspect mutable vs immutable state is also an important consideration –  jk. Aug 1 '12 at 9:34
    
Rule 8 describes the perfect process for creating designs that have thousands and thousands of classes which makes the system hopelessly complex, uncomprehendable and unmaintainable. But the plus side is that you get to follow SRP. –  Dunk Aug 1 '12 at 17:27
    
@Dunk I don't disagree with you, but that discussion is entirely off topic for the question. –  MattDavey Aug 1 '12 at 20:29
show 1 more comment

One possible implementation (in Java). I took liberties with the return types but over all I think it answers the question. TBH I don't think the interface to the Report class is that bad, though a better name might be in order. I left out guard statements and assertions for brevity.

EDIT: Also notice that the class is immutable. So once it is created you can't change anything. You could add a setFormatter() and a setPrinter() and not get into too much trouble. The key, IMHO, is to not change the raw data after instantiation.

public class Report
{
    private ReportData data;
    private ReportDataDao dao;
    private ReportFormatter formatter;
    private ReportPrinter printer;


    /*
     *  Parameterized constructor for depndency injection, 
     *  there are better ways but this is explicit.
     */
    public Report(ReportDataDao dao, 
        ReportFormatter formatter, ReportPrinter printer)
    {
        super();
        this.dao = dao;
        this.formatter = formatter;
        this.printer = printer;
    }

    /*
     * Delegates to the injected printer.
     */
    public void printReport()
    {
        printer.print(formatReport());
    }


    /*
     * Lazy loading of data, delegates to the dao 
     * for the meat of the call.
     */
    public ReportData getReportData()
    {
        if (reportData == null)
        {
            reportData = dao.loadData();
        }
        return reportData;
    }

    /*
     * Delegate to the formatter for formatting 
     * (notice a pattern here).
     */
    public ReportData formatReport()
    {
        formatter.format(getReportData());
    }
}
share|improve this answer
    
Thanks for the implementation. I have 2 things, in the line if (reportData == null) I presume you mean data instead. Secondly, I was hoping to know how u arrived to this implementation. Like why did you decide to delegate all calls to other objects instead. One more thing that I always wondered about, Is it really the responsibility of a report to print itself?! Why didn't you create a separate printer class that takes a report in its constructor? –  Songo Aug 7 '12 at 10:22
    
Yes, reportData = data, sorry about that. Delegation allows for fine-grained control of dependencies. At runtime you can provide alternate implementations for each component. Now you can have an HtmlPrinter, PdfPrinter, JsonPrinter,... etc. This is also handy for testing since you can test your delegated components in isolation as well as integrated in the object above. You could certainly invert the relationship between printer and report, I just wanted to show that it was possible to provide solution with the class interface provided. It's a habit from working on legacy systems.:) –  Heath Lilley Aug 7 '12 at 11:29
    
hmmmm... So if you were building the system from scratch, which option would you take? A Printer class that takes a report or a Report class that takes a printer ? I have encountered a similar problem before where I had to parse a report and I argued with my TL if we should build a parser that takes a report or whether the report should have a parser inside it and the parse() call is delegated to it. –  Songo Aug 7 '12 at 12:55
    
I would do both...printer.print(report) to start and report.print() if needed later. The great thing about the printer.print(report) approach is that it is highly reusable. It separates responsibility and it allows you to have convenience methods where you need them. Maybe you don't wan other objects in your system to have to know about the ReportPrinter, so by having a print() method on a class you are achieving a level of abstaction that insulates your report printing logic from the outside world. This still has a narrow vector of change and is easy to use. –  Heath Lilley Aug 7 '12 at 13:17
    
+1 I see. Thanks for the explanation. –  Songo Aug 7 '12 at 13:41
add comment

In your example, it is not clear SRP is being violated. Maybe the report should be able to format and print itself, if they are relatively simple:

class Report {
  void format() {
     text = text.trim();
  }

  void print() {
     new Printer().write(text);
  }
}

The methods are so simple it doesn't make sense to have ReportFormatter or ReportPrinter classes. The only glaring problem in the interface is getReportData because it violates ask don't tell on non-value object.

On the other hand, if the methods are very complicated or there are many ways to format or print a Report then it makes sense to delegate the responsibility (also more testable):

class Report {
  void format(ReportFormatter formatter) {
     text = formatter.format(text);
  }

  void print(ReportPrinter printer) {
     printer.write(text);
  }
}

SRP is a design principle not a philosophical concept and so it is based on the actual code you are working with. Semantically you can divide or group a class into as many responsibilities as you wish. However, as a practical principle, SRP should help you find the code you need to modify. Signs you are violating SRP are:

  • Classes are so large you waste time scrolling or looking for the right method.
  • Classes are so small and numerous you waste time jumping between them or finding the correct one.
  • When you have to make a change it affects so many classes it is hard to keep track.
  • When you have to make a change it is unclear what classes need to change.

You can fix these through refactoring by improving names, grouping similar code together, eliminating duplication, using a layered design, and splitting/combining classes as needed. The best way to learn SRP is to dive into a codebase and refactor away the pain.

share|improve this answer
    
could u plz check the example I attached to the post and elaborate your answer based on it. –  Songo Aug 6 '12 at 10:10
    
Updated. SRP depends on context, if you posted an entire class (in a separate question) it would be easier to explain. –  Garrett Hall Aug 6 '12 at 13:42
    
Thanks for the update. A question though, Is it really the responsibility of a report to print itself?! Why didn't you create a separate printer class that takes a report in its constructor? –  Songo Aug 7 '12 at 12:50
    
I'm just saying SRP depends on the code itself you shouldn't apply it dogmatically. –  Garrett Hall Aug 7 '12 at 13:37
    
yea I get your point. But if you were building the system from scratch, which option would you take? A Printer class that takes a report or a Report class that takes a printer ? Many times I'm faced with such a design question before figuring out whether the code will prove to be complex or not. –  Songo Aug 7 '12 at 13:44
show 1 more comment

Single Responsibility Principle is highly coupled with the notion of cohesion. In order to have a highly cohesive class you need to have co-dependence between the class' instance variables and its methods; that is, each of the methods should manipulate as many instance variables as possible. The more variables a methods uses, the more cohesive is to its class; maximum cohesion is usually unachievable.

Also, in order to apply SRP well you understand well the business logic domain; to know what each abstraction should do. Layered architecture is related to SRP also, by having each layer do a specific thing (Data Source Layer should provide data and so on).

Coming back to cohesion even if your methods do not use all the variables, they should be coupled:

public class MyClass {
    private Type1 var1;
    private Type2 var2;
    private Type3 var3;

    public Type3 method1() {
        //use var1 and var3
    }  

    public void method2() {
        //use var1 and var2
    }

    public Type1 method3() {
        //use var2 and var3
    }
}

You should not have something like the code bellow, where a part of the instance variables are used in a part of the methods, and the other part of the variables are used in the other part of the methods (here you should have two classes for each part of the variables).

public class MyClass {
    private Type1 var1;
    private Type2 var2;
    private Type3 var3;
    private TypeA varA;
    private TypeB varB;

    public Type3 method1() {
        //use var1 and var3
    }  

    public void method2() {
        //use var1 and var2
    }

    public TypeA methodA() {
        //use varA and varB
    }

    public TypeA methodB() {
        //use varA
    }
}
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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