ArticleS
.
DavidChelimsky
.
MattersOfPrinciple
.
SrpAppliedToMethods
Edit Page:
!title Single Responsibility Applied to Methods It has been said that SRP should apply to methods as well. This is most clearly expressed in the Command-Query Separation Principle, but I think it can be applied to any method. I read once (I wish I could remember where, and if any readers can point me in the right direction please, do) that a key difference between procedural and object oriented designs could be expressed like this: ''In procedural programming, a process is expressed in one place in which a series of instructions are coded in order. Whereas in OO, a process is expressed as a succession of messages across objects. One object sends a message to another, which does part of the process and then sends a new message off to another object, which handles part of the process, etc. You modify a process by reorganizing the succession of messages rather than changing a procedure.'' With that in mind, consider the following story: * A user can press a Save Button on the Person Screen, at which point the Person is saved to the database. The user is then notified as to the success of the Save. In a system I've been working on, we have code that handled this story something like this (language and details have been changed to protect the innocent - and to shamelessly plug [[rspec][http://rspec.rubyforge.org]]): {{{def save_requested save_result = persister.save person view.display save_result.message if save_result.success? view.alert save_result.message if save_result.failure? end }}} So the presenter defines a procedure, depending on the Persister and the !-SaveResult-! to make it happen. We like to use mocks in our executable specifications, so the specs for this method look something like this: {{{context "A person presenter" do setup do @person = Person.new @mock_persister = mock("persister") @mock_view = mock("view") @presenter = PersonPresenter.new(@person, @mock_persister, @mock_view) end specify "should delegate to the persister when it receives 'Save'" do @mock_persister.should_receive(:save).with(@person).and_return(!-SaveResult-!.new(:success)) @presenter.save_requested end specify "should tell view to display success message on successful 'Save'" do @mock_persister.should_receive(:save).with(@person).and_return(!-SaveResult-!.new(:success, "Success!")) @mock_view.should_receive(:display).with("Success!") @presenter.save_requested end specify "should tell view to alert with failure message on unsuccessful 'Save'" do @mock_persister.should_receive(:save).with(@person).and_return(!-SaveResult-!.new(:failure, "Failed!")) @mock_view.should_receive(:alert).with("Failed!") @presenter.save_requested end end}}} Note that in the first spec, "should delegate to the persister when it receives 'Save'", we need to set up the !-SaveResult-! to be returned even though we don't need it for the spec. We just need it in order for the code to run. This sort of thing has a way of gumming up specs ("tests" if you prefer) and making them more difficult to understand and more tightly bound to the code (and therefore more brittle - subject to changes in the code forcing changes in the specs). My team recently discussed a design change: {{{def save_requested persister.save person, self end def on_successful_save view.display save_result.message end def on_failed_save view.alert save_result.message end }}} Look at the the effect on the specs: {{{context "A person presenter" do setup do @person = Person.new @mock_persister = mock("persister") @mock_view = mock("view") @presenter = PersonPresenter.new(@person, @mock_persister, @mock_view) end specify "should delegate to the persister when it receives 'Save'" do @mock_persister.should_receive(:save).with(@person) @presenter.save_requested end specify "should tell view to display success message on successful 'Save'" do @mock_view.should_receive(:display).with("Success!") @presenter.on_successful_save(!-SaveResult-!.new(:success, "Success!")) end specify "should tell view to alert with failure message on unsuccessful 'Save'" do @mock_view.should_receive(:alert).with("Failed!") @presenter.on_failed_save(!-SaveResult-!.new(:failure, "Failed!")) end end}}} The context here is the same, but note that each spec becomes smaller, simpler, more focused, easier to understand, etc. See how we don't need to set up the !-SaveResult-! in "should delegate to the persister when it receives 'Save'"? Also note that the !-SaveResult-! constructor may no longer need :success or :failure because the Presenter (client of !-SaveResult-!) is not having to query for its state, thereby further reducing the coupling. There are a couple of other benefits as well. First, if the requirements about how to handle a successful Save change, this change should only affect the "should tell view to display success message on successful 'Save'" and !-Presenter#on_successful_save-!. So changes are better localized - a primary goal of most design principles. We've also reduced duplication in the system. We've done so by reducing the number of places that have to make decisions based on the state of successfulness of the Save. In the first example, the Persister (or some collaborator) has to decide how to construct the !-SaveResult-! based on whether it was successful or not AND the Presenter needs to figure out how to handle the !-SaveResult-! based on the same condition. In the latter example, this decision is only made once and arguably in the correct place (where the Save is happening). So even though we've added two methods to the Presenter, we've simplified the overall system by localizing change and reducing duplication, and we've made the specs easier to understand. Win, win, win. I guess that the underlying thought here is that methods have a lot in common with objects, and we should consider the class design principles when we structure our methods. I'm not saying this always works. I'm not saying it doesn't either. I'm just saying that it's worth considering. You may get a better design out of the deal. ---- !commentForm !* Sun, 4 Jun 2006 14:11:00, Dave Astels, Nicely done Good job! As for the quote.. hmm.. sounds like something Alan Kay or Kent Beck would have said... sounds like soemthing describing Smalltalk specifically rather than OO in usually implemented. Message passing & method invovation are different things.. but in this context that's pickign nits. *! !* Thu, 22 Jun 2006 07:50:18, Clint Shank, Quote You might be referring to the Tell, Don't Ask principle. In procedural coding, you call getters to get all the data, then operate on the data. A more OO way of accomplishing the same thing would be to tell other objects to do the work, distributing the work among neighboring objects, which themselves could do some work and distribute even further. *! !* Mon, 7 Aug 2006 16:22:40, nicholas evans, am I missing a detail in the final implementation? I've been trying to do TDD with ASP.Net using the Model View Presenter pattern, and I often run into the issues you describe here. My tests/specs often feel too course, testing multiple things at once (as in your original specs). I had thought that it was a limitation of the mock framework, but after reading this, it seems that perhaps my methods should try to follow the Single Responsibility Principle more closely. However, it seems to me that even though you have removed the duplication in your tests and seperated the responsibilities (good things), you are no longer testing everything that was previously tested. At least, not as you've written it here. :-) Someone still needs to be able to trigger the Presenter#on_successful_save, yes? If that responsibility is being removed from the Presenter, where is it being added? It is going to the persister, right? ("In the latter example, this decision is only made once and arguably in the correct place (where the Save is happening).") In the original specs, that behaviour was tested (albeit as a side effect) in the same place where the persistance was tested, the second and third specs. But in the new specs, that behaviour is completely missing. Presumably that behaviour moved into the persister... If so, what new specs must the persister obey in order preserve the original behaviour? Thanks. *! !* Thu, 10 Aug 2006 02:44:43, ${chelimsky}, re: am I missing a detail in the final implementation? Nicholas - I see what you're saying, but I think that the same specs are missing from the first example as well. Note that persister's role in the first example (returning a SaveResult in the correct state) is never tested. Presumably that is tested elsewhere. The second example simply removes any redundant decision making from the presenter. *! !* Mon, 14 Aug 2006 09:15:15, Mike Nichols, Subclass utilities Your point here would also seem to question the love affair for creating superclasses simply because a class family all need to share similar behavior (methods). In other words, I make the mistake of thinking that because a set of classes would need a common method I should consider inheritance, when in fact it would be better to encapsulate the Utility in its own Command or other way of avoiding subclassing. *! !* Mon, 14 Aug 2006 17:15:38, nicholas evans, Yes, the persister's role (both before and after the change) is presumably tested elsewhere. I think I'm just looking for more generic TDD/BDD/refactoring guidelines about how you would have effected this change: a clarification about how I should ensure the system behaviour is preserved. I'm still new at this. :-) In my way of looking at things, this is a refactoring at the system level (because the system as a whole should behave the same before and after the change), but at the unit level it is a test-breaking change in design. We're more than happy to loosen the specs for the presenter a little bit, because that keeps its design simple, makes cleaner and more obvious specs, DRYs out the system a bit (the redundant decision), it is supported by the SRP, etc. But in order for the system behaviour to remain the same, there would need to be at least two additional unit level specs: one for the persister (or someone) to send out the appropriate events or callbacks (I suppose that it's entirely possible that this behaviour is already defined for the persister prior to making the change), and one more for the presenter (or some other orchestrating object) to ensure that presenter's event handlers are wired up to the observed object. It's that second spec that seems to be missing from the example. Yes? *!
Hints:
Use alt+s (Windows) or control+s (Mac OS X) to save your changes. Or, tab from the text area to the "Save" button!
Grab the lower-right corner of the text area to increase its size (works with some browsers).