Can you take the single responsibility principle too far?

  • [アーティクル]

This morning, I had a conversation with a colleague of mine who has recently started using EDD about "exposing things just for the purpose of testing." Although I have run across the odd occasion where I may need to add a get property to a class in order to verify a value, usually I find this means that the class is doing too much.

Here's the scenario:

Imagine a command-line application that needs to perform several actions. Each action is responsible for parsing and validating the parameters for itself. One possible structure looks like this:

 public class PourWaterAction : IAction 
{
  PourWaterAction(string[] args)
  {
  }
 
  public bool ValidateParamters() 
  {
     // throws if invalid 
  } 

  public void Perform() 
  { 
    // do real stuff here 
  }
}

Now we've got tests that will assert if ValidateParameters throws, but how do we write a test that determines if the parameters are properly parsed when ValidateParameters doesn't throw?

This is where the interesting conversation starts: my feeling was that the PourWaterAction class shouldn't be doing two things, and validation should be delegated to a different class. Perhaps something like this:

 public static class PourWaterActionValidator
{
  public static PourWaterData Validate()  
  {
    // Either throw or return a populated structure
  }
}

or

 public class PourWaterActionValidator : IActionValidator
{
  public PourWaterData Validate()  
  {
    // Either throw or return a populated structure
  }
}

Now we can move the validation tests to the new class, and easily write a test for passing conditions as well as failing ones. Straightforward enough, right?

Possibly not. My colleague was of the opinion that: 

  • This was creating extra classes unnecessarily which, when taken to the extreme, would mean tons of different files in the project.
  • Even if another class was created, it should be visible only to PourWaterAction which means it wouldn't be possible to test either.

This means that the only way to test it was to add get properties to PourWaterAction so the tests could verify that the parsing had been performed correctly — "adding things just for the sake of testing"

So, what would you do in this situation? And do you believe that classes should always strive to do one thing and one thing only?

Comments

  • Anonymous
    March 28, 2008
    I've labored over your question for literally MINUTES, and I think I have the answer.Here goes.  Can you take the single responsiblilty principle too far?"Yes.  Or no."Hope that helps.
  • Anonymous
    March 28, 2008
    Yes, the principle can definitely be taken too far, and I agree with your colleague in this case. For classes, I think the proper focus is not it should "do one thing" but instead "have one responsibility," but the performance of that responsibility can involve many factors. I would say your original PourWaterAction class meets that test. That is, it manages a particular action, including validating its parameters and performing the action. Those two functions are tightly bound and belong together.Methods should more clearly "do one thing," but here again it can be taken too far. I occasionally hear a dogma that methods should never be more than 20 lines, and you should break up larger methods. But that's an arbitrary number and violates the 0-1-infinity principle. I believe a method should perform one logical action, even if it takes 200 lines to do it. Any subparts independently useful should be factored out, of course, but if a subpart only makes sense within the context of that function, then it only adds complexity to separate it to a new method.
  • Anonymous
    March 28, 2008
    Keith - how do you go about properly testing the class if it takes care of validation and performing an action? Do you really just create get properties for the sole purpose of being able to verify that the validation worked?
  • Anonymous
    March 28, 2008
    Paul - how about another question then, in this case do you think the class should be split in two?
  • Anonymous
    March 28, 2008
    I agree with Keith.Concerning how to test parsing of input, you'd need to identify all the Equivalence Classes that correspond to your Boundary Values, and then test each scenario by itself (or, if you feel like being thorough, all permutations).If this is is difficult to set up (e.g. complex string concatenation of the input parameters), you should create a Test-Specific API for use in your tests.Regarding validation, it really depends on what it is that you need to test, but under the assumption that a method does SOME work, it's only a matter of design how you verify that work: Verification can happen on an object's post-test state, Direct, or Indirect Output, although you might need to tweak your design to enable interception of Indirect Output (i.e. use IoC/DI).In the case of a console application, my first design thought would probably be to let the application itself act as a Humble Executable that delegates all work to a testable library, but for an example of how to test the Indirect Output of a console application without such indirection, see my post at http://blogs.msdn.com/ploeh/archive/2006/10/21/ConsoleUnitTesting.aspx
  • Anonymous
    October 17, 2008
    Well, from my point of view, It is not a good design to make a object who performe the "ACTION" to parse the input parameter in the first place. Hence it leads to the diffculty of the "TEST"It looks like there are some advantages such that: we just give the input to each object, they understand it and then do the job accordingly. And When new "ACTION" is come in, new parse method is automatic added in.However, this lead to the problem: how the different "ACTION" object understanding the input parameters.Some question: 1) How to decide to which the input should be send (i.e., the sequence order of ACTION) Since only "ACTION" object understand the input parameter 2) what happens if the two "ACTION" require the same parameters? this lead to the problem that if one change it way of phase, then all the other need change also.Actually, the original design has departed from the principle "single responsibility", as parsing the input and do the action is actually tow responsibility.I think the "right" should be a class which is dedicated to deal with the input phrase and init the "ACTION" object, and each "action" class publish its "input data structure" (and from the point view of efficience, it is better to public the valid method as static). The phrase object will fill the "data" according to the input, and call the valid of action class to check if the input is ok and init the related objects.