73

I'm working on a team where the team leader is a virulent advocate of SOLID development principles. However, he lacks a lot of experience in getting complex software out of the door.

We have a situation where he has applied SRP to what was already quite a complex code base, which has now become very highly fragmented and difficult to understand and debug.

We now have a problem not only with code fragmentation, but also encapsulation, as methods within a class that may have been private or protected have been judged to represent a 'reason to change' and have been extracted to public or internal classes and interfaces which is not in keeping with the encapsulation goals of the application.

We have some class constructors which take over 20 interface parameters, so our IoC registration and resolution is becoming a monster in its own right.

I want to know if there is any 'refactor away from SRP' approach we could use to help fix some of these issues. I have read that it doesn't violate SOLID if I create a number of empty coarser-grained classes that 'wrap' a number of closely related classes to provide a single-point of access to the sum of their functionality (i.e. mimicking a less overly SRP'd class implementation).

Apart from that, I cannot think of a solution which will allow us to pragmatically continue with our development efforts, while keeping everyone happy.

Any suggestions ?

10
  • 21
    That's just my opinion, but I think there's one more rule, that is very easily forgotten under the pile of various acronyms - "Common Sense Principle". When a 'solution' creates more problems that it really solves, then something is wrong. My take is that if a problem is complex, but is enclosed in a class that takes care of its intricacies and is still relatively easy to debug - I'm leaving it alone. Generally your 'wrapper' idea seems sound to me, but I'll leave the answer to someone more knowledgeable. Commented May 29, 2012 at 11:14
  • 6
    As for 'reason to change' - there is no need to speculate on all the reasons prematurely. Wait until you actually have to change that, and then see what can be done to make such kind of change easier in the future.
    – driushkin
    Commented May 29, 2012 at 11:19
  • 65
    A class with 20 constructor parameters doesn't sound very SRP to me!
    – MattDavey
    Commented May 29, 2012 at 12:14
  • 1
    You write "... IoC registration and resolution ..."; this sounds as if you (or your team lead) thinks "IoC" and "dependency injection" (DI) are the same thing, which is not true. DI is a means to achieve IoC, but certainly not the only one. You should carefully analyze why you want to do IoC; if it is because you want to write unit tests, then you also could try to use the service locator pattern or simply interface classes (ISomething). IMHO, these approches are much easier to handle than dependency injection and result in more readable code.
    – user27363
    Commented May 29, 2012 at 13:13
  • 2
    any answer given here would be in a vacuum; we'd have to see the code to give a specific response. 20 parameters in a constructor? well, you might be missing an object... or they might all be valid; or they might belong in a config file, or they might belong in a DI class, or... The symptoms certainly sound suspicious, but like most things in CS, "it depends"... Commented Jun 5, 2012 at 20:09

8 Answers 8

95
+50

If your class has 20 parameters in the constructor, it doesn't sound like your team quite knows what SRP is. If you have a class that does only one thing, how does it have 20 dependencies? That's like going on a fishing trip and bringing along a fishing pole, tackle box, quilting supplies, bowling ball, nunchucks, flame thrower, etc.... If you need all that to go fishing, you're not just going fishing.

That said, SRP, like most principles out there, can be over-applied. If you create a new class for incrementing integers, then yeah, that may be a single responsibility, but come on. That's ridiculous. We tend to forget that things like the SOLID principles are there for a purpose. SOLID is a means to an end, not an end in itself. The end is maintainability. If you are going to get that granular with the Single Responsibility Principle, it's an indicator that zeal for SOLID has blinded the team to the goal of SOLID.

So, I guess what I'm saying is... The SRP isn't your problem. It's either a misunderstanding of the SRP, or an incredibly granular application of it. Try to get your team to keep the main thing the main thing. And the main thing is maintainability.


Get people to design modules in a way that encourages ease of use. Think of each class as a mini API. Think first, "How would I like to use this class," and then implement it. Don't just think "What does this class need to do." The SRP does have a great tendency to make classes harder to use, if you don't put much thought into usability.


If you're looking for tips on refactoring, you can start doing what you suggested - create coarser-grained classes to wrap several others. Make sure the coarser-grained class is still adhering to the SRP, but on a higher level. Then you have two alternatives:

  1. If the finer-grained classes are no longer used elsewhere in the system, you can gradually pull their implementation into the coarser-grained class and delete them.
  2. Leave the finer-grained classes alone. Perhaps they were designed well and you just needed the wrapper to make them easier to use. I suspect this is the case for much of your project.

When you're finished refactoring (but before committing to the repository), review your work and ask yourself if your refactoring was actually an improvement to maintainability and ease of use.

8
  • 3
    Alternative way to get people think about designing classes: let them write CRC-cards (Class Name, Responsibility, Collaborators). If a class has too many collaborators or responsibilities, it most likely isn't SRP-ish enough. In other words, all the text has to fit in the index card, or else it is doing too much.
    – Spoike
    Commented May 30, 2012 at 10:59
  • 20
    I know what the flamethrower is for, but how the heck do you fish with a pole? Commented May 30, 2012 at 14:17
  • 13
    +1 SOLID is a means to an end, not an end in itself.
    – B Seven
    Commented May 31, 2012 at 13:55
  • 1
    +1: I've argued before that things like "The Law of Demeter" are misnamed, it should be "The Guide line of Demeter". These things should work for you, you shouldn't be working for them. Commented Jun 1, 2012 at 8:46
  • 2
    @EmmadKareem: It is true that DAO objects are supposed to have several properties. But then again, there are several things that you can group together in something as simple as a Customer class and have more maintainable code. See examples here: codemonkeyism.com/…
    – Spoike
    Commented Jun 8, 2012 at 5:42
35

I think it's in Martin Fowler's Refactoring that I once read a counter-rule to SRP, defining where it's going too far. There is a second question, as important as "does every class have only one reason to change?" and that is "does every change only affect one class?"

If the answer to the first question is, in every case, "yes" but the second question is "not even close," then you need to look again at how you're implementing SRP.

For example, if adding one field to a table means you have to change a DTO and a validator class and a persistence class and a view model object and so on then you've created a problem. Maybe you ought to rethink how you've implemented SRP.

Perhaps you've said that adding a field is the reason to change the Customer object, but changing the persistence layer (say from an XML file to a database) is another reason to change the Customer object. So you decide to create a CustomerPersistence object as well. But if you do it such that adding a field STILL requires a change to the CustomerPersisitence object then what was the point? You've still got an object with two reasons to change -- it just isn't Customer any more.

However, if you introduce an ORM, it's quite possible that you can make the classes work such that if you add a field to the DTO, it will automatically change the SQL used to read that data. Then you've got good reason to separate the two concerns.

In summary, here's what I tend to do: if there is a rough balance between the number of times I say "no, there is more than one reason to change this object" and the number of times I say "no, this change will affect more than one object," then I think I have the balance right between SRP and fragmentation. But if both are still high then I start to wonder if there's a different way that I can separate concerns.

3
  • +1 for "does every change only affect one class?"
    – dj18
    Commented Mar 12, 2014 at 19:34
  • A related issue I've not seen discussed is that if tasks that are tied to one logical entity get fragmented among different classes, then it may be necessary for code to hold references to multiple distinct objects which are all tied to the same entity. Consider, for example a kiln with functions "SetHeaterOutput" and "MeasureTemperature". If the kiln were represented by independent HeaterControl and TemperatureSensor, objects, then nothing would prevent a TemperatureFeedbackSystem object from holding a reference to one kiln's heater and a different kiln's temperature sensor.
    – supercat
    Commented Oct 11, 2014 at 16:49
  • 1
    If instead those functions were combined into an IKiln interface, which was implemented by a Kiln object, then the TemperatureFeedbackSystem would only need to hold a single IKiln reference. If it was necessary to use a kiln with an independent aftermarket temperature sensor, one could use a CompositeKiln object whose constructor accepted an IHeaterControl and ITemperatureSensor and used them to implement IKiln, but such deliberate loose composition would be easily recognizable in code.
    – supercat
    Commented Oct 11, 2014 at 16:52
24

Just because a system is complex doesn't mean that you have to make it complicated. If you have a class that has too many dependencies (or Collaborators) like this:

public class MyAwesomeClass {
    public class MyAwesomeClass(IDependency1 _d1, IDependency2 _d2, ... , IDependency20 _d20) {
      // Assign it all
    }
}

...then it got way too complicated and you're not really following SRP, are you? I'd bet if you wrote down what MyAwesomeClass does on a CRC card it wouldn't fit on an index card or you have to write in really tiny illegible letters.

What you have here is that your guys only followed the Interface Segregation Principle instead and may have taken it to an extreme but that is a whole other story. You could argue that the dependencies are domain objects (which happens) however having a class that handles 20 domain objects at the same time is stretching it a bit too far.

TDD will provide you a good indicator of how much a class does. Bluntly put; if a test method has setup code that takes forever to write (even if you refactor the tests) then your MyAwesomeClassprobably has too much things to do.

So how do you solve this conundrum? You move the responsibilities to other classes. There are some steps you can take on a class that has this problem:

  1. Identify all the actions (or responsibilities) that your class does with it's dependencies.
  2. Group the actions according to closely related dependencies.
  3. Redelegate! I.e. refactor each of the identified actions to either new or (more importantly) other classes.

An abstract example on refactoring responsibilities

Let C be a class that has several dependencies D1, D2, D3, D4 that you need to refactor to use less. When we identify what methods that C calls on the dependencies we can make a simple list of it:

  • D1 - performA(D2), performB()
  • D2 - performD(D1)
  • D3 - performE()
  • D4 - performF(D3)

Looking at the list we can see that D1 and D2 are related to each other as the class need them together somehow. We can also see that D4 needs D3. So we have two groupings:

  • Group 1 - D1 <-> D2
  • Group 2 - D4 -> D3

The groupings are an indicator that the class now has two responsibilities.

  1. Group 1 - One for handling the calling two objects that need each other. Maybe you can let your class C eliminate the need of handling both dependencies and leave one of them handling those calls instead. In this grouping, it is obvious that D1 could have a reference to D2.
  2. Group 2 - The other responsibility needs one object to call another. Can't D4 handle D3 instead of your class? Then we probably can eliminate D3 from the class C by letting D4 do the calls instead.

Don't take my answer as set in stone as the example is very abstract and makes a lot of assumptions. I'm pretty sure there are more ways to refactor this, but at least the steps might help you get some kind of a process to move responsibilities around instead of splitting classes up.


Edit:

Among the comments @Emmad Karem says:

"If your class has 20 parameters in the constructor, it doesn't sound like your team quite knows what SRP is. If you have a class that does only one thing, how does it have 20 dependencies? " - I think that If you have a Customer class, it is not strange to have 20 parameters in the constructor.

It is true that DAO objects tend to have a lot of parameters, which you have to set in your constructor, and the parameters are usually are simple types such as string. However in the example of a Customer class, you can still group it's properties inside other classes to make things simpler. Such as having an Address class with streets and a Zipcode class that contains the zipcode and will handle business logic such as data validation as well:

public class Address {
    private String street1;
    //...

    private Zipcode zipcode;

    // easy to extend
    public bool isValid() {
        return zipcode.isValid();
    }
}

public class Zipcode {
    private string zipcode;
    public bool isValid() {
        // return regex match that zipcode contains numbers
    }
}

This thing is discussed further in the blog post "Never, never, never use String in Java (or atleast often)". As an alternative of using constructors or static methods to make the sub objects easier to create you could use a fluid builder pattern.

3
  • +1: Great answer! Grouping is IMO a very powerful mechanism because you can apply grouping recursively. Speaking very roughly, with n abstraction layers you can organize 2^n items.
    – Giorgio
    Commented May 31, 2012 at 20:31
  • +1: Your first few paragraphs sum up exactly what my team is facing. "Business Objects" that are actually service objects, and unit test setup code that is mind numbing to write. I knew we had an issue when our service layer calls would contain one line of code; a call to a business layer method. Commented Oct 12, 2015 at 14:02
  • I looked at the “never use string” article and first thing he has a “name” and a “firstname” instance variable of type “Name” and “Firstname”. Which is an awful example of encapsulation gone wrong. There should be one object “Name” with family name, given name (not! First name), other first names, middle names, initials, address (Mr, Mrs, Prof. Dr. Dr etc) part of the name class.
    – gnasher729
    Commented Mar 14, 2021 at 14:50
7

A lot of the answers here are really good but are focused on the technical side of this issue. I'll simply add that it sounds like the developer's attempts to follow the SRP actually violate the SRP.

You can see Bob's blog here on this situation, but he argues that if a responsibility is smeared across multiple classes then the SRP is violated because those classes change in parallel. I suspect your dev would really like the design at the top of Bob's blog, and might be a bit disappointed to see it ripped apart. In particular because it violates the "Common Closure Principle" - things that change together stay together.

Remember that the SRP refers to "reason for change" and not "do one thing", and that you don't need to concern yourself with that reason for change until a change actually occurs. The second guy pays for abstraction.

Now there's the second problem - the "virulent advocate of SOLID development." It sure doesn't sound like you have a great relationship with this developer, so any attempts to convince him/her of the problems in the codebase are stumped. You'll need to repair the relationship so you can have a real discussion of the issues. What I would recommend is beer.

No seriously - if you don't drink head to a coffee shop. Get out of the office and somewhere relaxed, where you can talk about this stuff informally. Rather than trying to win an argument at a meeting, which you won't, have a discussion somewhere fun. Try to recognize that this dev, who is driving you nuts, is an actual functioning human who is trying to get software "out the door" and doesn't want to ship crap. Since you likely share that common ground, you can start to discuss how to improve the design while still conforming to the SRP.

If you can both acknowledge that the SRP is a good thing, that you just interpret aspects differently, you can probably start to have productive conversations.

2
  • some real life lessons along with software advice.. like it Commented Mar 11, 2021 at 17:14
  • Beer works really well if the team lead doesn’t drink :-)
    – gnasher729
    Commented Mar 14, 2021 at 14:43
4

I agree with all the answers about SRP and how it can be taken too far. In your post you mention that due to "over-refactoring" to adhere to SRP you found encapsulation breaking or being modified. The one thing that has worked for me is to always stick to the basics and do exactly what is required to meet an end.

When working with Legacy systems the "enthusiasm" to fix everything to make it better is usually quite high in Team Leads, especially those that are new to that role. SOLID, just doesn't have SRP - That's just the S. Make sure that if you are following SOLID, you don't forget the OLID as well.

I am working on a Legacy system right now and we started going down a similar path in the beginning. What worked for us was a collective team decision to make the best of both worlds - SOLID and K.I.S.S (Keep It Simple Stupid). We collectively discussed major changes to code structure and applied common sense in applying various development principles. They are great as guidelines not "Laws of S/W Development". The team is not just about the Team Lead - its about all the developers on the team. The thing that has always worked for me is to get everyone in a room and come up with a shared set of guidelines your entire team agrees to follow.

Regarding how to fix your current situation, if you use a VCS and have not added too many new features to your application, you can always go back to a version of code the entire team thinks is understandable, readable and maintainable. Yes! I am asking you to throw away work and start from scratch. This is better than trying to "fix" something that was broken and move it back to something that already existed.

1
  • When the team leader has broken the project, going back to an unbroken state is likely the best solution.
    – gnasher729
    Commented Mar 13, 2021 at 10:46
4

The answer is maintainability and clarity of code above all else. For me that that means writing less code, not more. Less abstractions, less interfaces, less options, less parameters.

Whenever I evaluate a code restructure, or adding a new feature I think about how much boilerplate will be required compared to the actual logic. If the answer is more than 50%, it probably means I'm over thinking it.

On top of SRP, there are many other development styles. In your case its sounds like YAGNI is definitely lacking.

0

The single responsibility principle has probably caused more damage than any other design principle, and when SRP meets “virulent advocate” you are in trouble. It seems your advocate has gone completely off the rails and decided that applying misunderstood principles is more important than a working and maintainable software.

The biggest problem is people confusing “single responsibility” with “single task”. If your Person object supports name, first name, and date of birth, they see five responsibilities (three for day, month and year of birth), and it looks that’s your problem here.

Your team lead has torn everything apart into little pieces, and you suggest putting the little pieces into suitable boxes. That may be the best in the circumstances, but it’s obviously awful - the real change would have to be a level higher. Undoing the damage that your team lead is doing.

-1

I agree with your team-lead decision [update=2012.05.31]that SRP is generally a good think. But i totally agree to @ Spoike -s comment that a constructor with 20 interface arguments is far to much.[/update]:

Introducing SRP with IoC moves complexety from one "multi-responsible-class" to many srp-classes and a much more complicated initialisation for the benefit of

  • easier unit-testability/tdd (testing one srp-class in isolation at a time)
  • but at the cost of
    • a much more difficuilt code initialisation and integration and
    • more difficuilt debugging
    • fragmentation (= distribution of code over several files/directories)

I am afraid you cannot reduce codefragmentation without sacrifying srp.

But you can "ease the pain" of codeinitialisation by implementing a syntactic sugar class that hides the complexity of initialisation in a constructor.

   class MySrpClass {
      MySrpClass(Interface1 parm1, Interface2 param2, .... Interface20 param2) {
      }
   } 

   class MySyntaxSugarClass : MySrpClass {
      MySyntaxSugarClass() {
         super(new MyInterface1Implementation(), new MyImpl2(), ....)
      }
   }
2
  • 2
    I believe 20 interfaces is an indicator that the class has too much to do. I.e. there are 20 reasons for it to change, which is pretty much a violation of SRP. Just because the system is complex doesn't mean that it has to be complicated.
    – Spoike
    Commented May 30, 2012 at 11:06
  • I think the twenty interfaces are more an indication that the interfaces have been split to an absolutely ridiculous degree. Where I fully agree is that if you have a twenty or even ten interfaces, then something has gone badly wrong and needs fixing.
    – gnasher729
    Commented Mar 14, 2021 at 14:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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