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):

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