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.

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 ?

share|improve this question

migrated from stackoverflow.com May 29 '12 at 23:17

This question came from our site for professional and enthusiast programmers.

13  
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. –  Patryk Ćwiek May 29 '12 at 11:14
5  
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 May 29 '12 at 11:19
48  
A class with 20 constructor parameters doesn't sound very SRP to me! –  MattDavey May 29 '12 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 May 29 '12 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"... –  Steven A. Lowe Jun 5 '12 at 20:09

6 Answers 6

up vote 54 down vote accepted
+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.

EDIT

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.

EDIT 2

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.

share|improve this answer
2  
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 May 30 '12 at 10:59
13  
I know what the flamethrower is for, but how the heck do you fish with a pole? –  R. Martinho Fernandes May 30 '12 at 14:17
8  
+1 SOLID is a means to an end, not an end in itself. –  B Seven May 31 '12 at 13:55
    
+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. –  Binary Worrier Jun 1 '12 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 Jun 8 '12 at 5:42

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.

share|improve this answer
    
+1 for "does every change only affect one class?" –  dj18 Mar 12 at 19:34

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.

share|improve this answer
    
+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 May 31 '12 at 20:31

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.

share|improve this answer

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.

share|improve this answer

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(), ....)
      }
   }
share|improve this answer
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 May 30 '12 at 11:06

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.