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.
Honestly, I think you're taking the concept of single responsibility a little too far. Getters and setters are incidental to the functioning of the class whether you do it by direct access to public members or use methods (or properties) to do it.
You're making the argument that getting and setting some member of the class is a separate responsibility and should therefore be moved elsewhere. Let's say we do that, and now you have classes called Config
and ConfigAccessor
. At this point, you now have an air gap between the two classes, because Config
has no interface to access its location
member. That makes it impossible to write ConfigAccessor
, and you're left with an immutable, write-once class that's of no use whatsoever. If you add some sort of interface to allow ConfigAccessor
to do its job, you'll find yourself with a recursive problem.
The SRP, like many other things in this field is a principle, not a hard-and-fast rule. That means you should be applying judgement to your situation instead of trying to follow it unconditionally. There's a line between being a purist and getting the job done, and when the former is preventing the latter, you're on the wrong side of it.
If I can critique your design a bit: If your Config
class is designed to be an interface between a configuration file stored on disk and your code, the last thing you want to do is change its location midstream. If you're changing the location
as a way of starting access to a different file, you should be destroying the old object and creating a new one. You didn't make clear whether you intend to store the contents of the file in the object. If you'd planned to use it as a way to inhale the contents of one configuration file and write it to another, consider using a method that clones the data into a new object that points at the new file.
Best Answer
YAGNI means to avoid investing effort into code changes for hypothetical requirements which may arrive later, and instead focus on the requirements one has now. But this is not restricted to functional requirements - as long as one does not create "use-once-and-then-throw away" software, there is always the non-functional requirement of keeping code readable, understandable and evolvable. And separating responsibilities is one of the major tools to achieve that goal.
Hence I interpret the YAGNI principle in such a situation as a recommendation for not separating responsibilities before the benefits of the SRP become visible, which is actually as soon as the code becomes convoluted. And that usually happens very quickly when one tries to implement different business requirements in one class.
I would tolerate it if I had to make two or three small extensions to the "class where the workflow is implemented" to add this logging requirement. But then my pain level would probably be reached, and I would start thinking "heck, can we refactor this logging mechanism, or at least the data collection out of the workflow class to a better place".
So instead of telling your devs:
tell them:
That should be the justification one needs to apply the SRP not for some unknown requirement in the future, but for the requirement of keeping the code understandable now.
Of course, the threshold where code is perceived as being convoluted, and when the SRP could be applied to fix this may vary from one dev to another, but that is balance your team has to find, and where only code reviews can help.