Clarifying Business Logic and Writing Clean Code

Summary

In this article I explore the relative merits of Acceptance testing in Cucumber versus RSpec, the process of making an application's business logic explicit in acceptance tests, and explicit business logic in the application code itself. These topics are considered in the context of analyzing a Cucumber feature from a real project. I conclude that there is a strong parallel between the tension of summarising versus making things explicit at both the acceptance and application level. Furthermore, I suspect the balance of this tension will always depend on circumstances.


So I'm mulling over a set of scenarios we have for our non-profit directory web site. These scenarios are centred around unclaimed non-profits (or organisations) that we have in our directory who do not yet have a registered user as an admin. We have a number of these organisations in the system that we generated from government data on non-profits in the local area. There are a number of ways that a user of the web site can become an "organisation admin" as we call the users who are permitted to edit the non-profit details and post volunteer opportunities. One of these is called the "This Is My Organisation" path or TIMO for short. All of the organisations in our directory have a profile page which display a prominent button with the words "This is my organisation". The idea is that a user clicks on this to indicate that they are an administrator of that particular organisation.

this is my org button

So far so good. Complexities arise in that we don't want to let just anyone become able to edit an organisation's details or post volunteer opportunities, so these requests need to be approved by the site administrator. In addition we want to let the user know that their request has been recorded, and that the site administrator has been notified. We also want to remove the TIMO button as a further indicator to the user that they have already sent a request. Our objective is to make the user experience as smooth as possible for both the individual requesting organisation admin permissions and the site administrator who is handling the requests. For the moment let us focus on the individual user; and before getting into the technical details a little more background on the project. Ours is an open source project that has been worked on by many different developers and so there is a mix of styles in the codebase, but we follow the agile methodology of regular meetings with the customer, user stories, BDD/TDD and pair programming. Currently we have our acceptance tests in Cucumber, unit/functional tests in RSpec, and the system is Rails 3.2 on Ruby 1.9.

Here's the first scenario in our "TIMO" this-is-my-org.feature cucumber file

  Scenario: I am a signed in user who requests to be admin for my organisation
    Given I am signed in as a non-admin
    When I visit the show page for the organisation named "Helpful Folk"
    And I click "This is my organisation"
    Then I should be on the show page for the organisation named "Helpful Folk"
    And I should not see "This is my organisation"
    And "nonadmin@myorg.com"'s request status for "Helpful Folk" should be updated appropriately
    And an email should be sent to "admin@myorg.com" as notification of request

This whole feature has been through a number of revisions. An initial comprehensive solution was broken up into a series of smaller components that used a more RESTful approach that separated out the different concerns. In the original solution custom controller actions ('admin_request' rather than 'update') did all the work. The problem with this was a large pull request that was difficult to digest, and a complex feature that would be difficult to maintain. Furthermore the routes were not very RESTful, particularly given that the underlying actions are just changes to resources. Two things that we removed for later addition were email notifications to the site administrator and being able to persist the request data across logins and registrations. It's our recent work adding those two components back in that have led us to take another look at this feature.

The way the system currently supports this first scenario is that the request goes to a UserController which kicks off a UserOrganisationClaimer service which then calls back to the UserController to display a flash message to the user, send email to admin; and makes a request to the user model to update a pending_organisation_id attribute which is stored in the database as a permanent record of the event.

UserOrganisationClaimer Service

The UserOrganisationClaimer service is designed to extract the business logic and make it explicit and independent of the Rails framework. It has the unfortunate side effect of making a simple set of steps rather more convoluted. Interestingly there is a parallel in the Cucumber scenario itself. One of my concerns on reviewing the "I am a signed in user who requests to be admin for my organisation" scenario introduced earlier is that is somewhat imperative, i.e. it is describing the steps through the interface (visiting and clicking) rather than a more declarative statement of the business logic. In addition the one very declarative statement And "nonadmin@myorg.com"'s request status for "Helpful Folk" should be updated appropriately is rather vague. In an attempt to improve the scenario I re-wrote it as follows:

  Scenario: Signed in non-site-admin requests to be admin of an organisation
    Given I am signed in as a non-admin
    When I click "This is my organisation" on the "Helpful Folk" page 
    Then my request for "Helpful Folk" should be persisted
    And I should see "You have requested admin status for Helpful Folk"
    And I should not see a link or button "This is my organisation"
    And an email should be sent to "admin@myorg.com" as notification of the request

I have not entirely convinced myself that this is a huge improvement, but it does seem to me that the initial 'visit', 'click' and 'page' steps do not deserve individual lines, and replacing them with a single line makes sense. We could make an even shorter scenario such as:

  Scenario: Signed in non-site-admin requests to be admin of an organisation
    Given I am signed in as a non-admin
    When I click "This is my organisation" on the "Helpful Folk" page 
    And "nonadmin@myorg.com"'s request status for "Helpful Folk" should be updated appropriately    
    And an email should be sent to "admin@myorg.com" as notification of the request

but I worry about concealing several of the expected consequences inside a step definition that refers to something being 'updated appropriately'. "When I click" is still rather imperative and could perhaps be changed to "When I request", however the click makes it clear that the current interface involves a link or button, rather than say, a mouseover or a drag and drop; although again, arguably the business logic description should be free from interface specifics since they may well change in future. Then again, knowledge of the product and customer suggests the chance of this is relatively low.

It seems clear that the same standards that we apply to code methods and their naming should equally apply in Cucumber. The Scenario name, in this case 'Signed in non-site-admin requests to be admin of an organisation', is the equivalent of a method name and needs to summarise the scenario effectively. Each of the individual steps is a method call, and each needs to be comprehensible. As when writing application code there is a tension between concealing details and making things explicit. The question becomes what are the important details to expose at any particular level? Business logic or UI/implementation details? One clear advantage of Cucumber scenarios is that being able to use spaces between words makes the language much more readable. We can have exactly the same sort of acceptance test in RSpec, but we would be forced to insert various punctuation like so:

scenario "Signed in non-site-admin requests to be admin of an organisation" do
  sign_in_as_non_admin
  click_TIMO_button_on_page 'Helpful Folk'
  expect(request_status('nonadmin@my_org.com')).to be_updated 'Helpful Folk'
  expect(email_for('admin@myorg.com')).to be_sent
end

Or something similar. One could go to some lengths to write a Ruby DSL that would support even more readable Ruby code, but Cucumber sidesteps that issue by using the Gherkin format. We can argue back and forth about RSpec versus Cucumber, but I think the fundamental tension at the level of our business logic remains; i.e. how much and what to expose, and what to summarise and hide? RSpec has the advantage that we can write directly in Capybara. Conversely with Cucumber, step definitions will conceal the Capybara commands that we see in RSpec scenarios. Cucumber forces at least one additional layer of abstraction upon us. We can chose to introduce that level of abstraction in RSpec, just as we can choose to introduce additional layers of abstraction in our application code. One gets additional readability in Cucumber at the expense of another level of abstraction. Abstraction is very powerful, but is a double edged sword. For those trying to maintain an application is it more important that they have a clear abstraction of the precise business logic independent of any interface or application logic? Or should parts of the interface/application show through in order to make it clear how things work now, independently of the high level abstraction?

Speaking of application code, I described above the UserOrganisationClaimer service which supports the functionality tested by the scenarios above. The intention, apart from reusability is, I believe, to make the business logic explicit and independent of application framework specifics. Let's look at the UserOrganisationClaimer; it has a high level method that summarises it's overall behaviour:

  def call(organisation_id)
    request_admin_status_if(organisation_id)
    error_message_if_not_admin_or_not(organisation_id)
    promote_user_if_admin_and_not(organisation_id)
  end

it introduces some additional logic to support a second scenario where an admin is approving an earlier user request. It is called from the UserController 'update' method like so:

  def update
    usr = User.find_by_id params[:id]
    UserOrganisationClaimer.new(self, usr, usr).call(params[:pending_organisation_id])
  end

  def update_message_for_admin_status
    org = Organisation.find(params[:pending_organisation_id])
    flash[:notice] = "You have requested admin status for #{org.name}"
    redirect_to organisation_path(params[:pending_organisation_id])
  end

and in the code segment above we see also an 'update_message_for_admin_status' hook which gets called by the UserOrganisationClaimer 'request_admin_status_if' method. This allows rails specifics such as the flash and path redirection to be removed from the 'business logic' of the UserOrganisationClaimer. The UserOrganisationClaimer service itself was subject to many rounds of revisions and refactoring and at one stage was a whole collection of services. The collection of services seemed to me like unwarranted complexity (even if very flexible) and so the logic was refactored into a single service. At the time, the 'call' method seemed like a good compromise in terms of readability and complexity; however upon reviewing the service after three months it seemed rather difficult to interpret without having worked with it before. To write the 'call' method into English we would say perhaps:

  1. Request Org Admin status for specified User IF an Organisation is specified
  2. Present an error message IF an Organisation is not specified OR the current user is not an Admin
  3. If the current user is an Admin AND an Organisation is specified THEN promote the specified User to Org Admin

The method names in 'call' are chosen to embody these concepts, but it still feels convoluted. It definitely feels like two jobs are being performed. Perhaps that implies we should go back to multiple services. Originally all of this logic was in the controller and it was convoluted there - hence the pressure to refactor. What is particularly interesting is the parallel between the description of the business logic in both the Cucumber scenario and the service. One major difference is that the service conflates at least two user stories so it is more complex, but the attempt of both to raise the level of abstraction leads naturally to the question of the necessity of specifying the same abstracted business logic in two places (acceptance test and application code). The response might be that the Cucumber acceptance test is providing a clarity for the nontechnical designer or customer who wishes to understand the business logic, while the service internals are attempting to provide a similar clarity, but for a technical user. However it still feels somewhat unDRY.

In attempting to unravel all this I have been tempted to move some of the service internals back into the controller and operate in a more RESTful form; to the extent that REST specifies anything beyond the design of HTTP requests, e.g. 'PUT /user/7?pending_organisation_id=12' in this case, to imply that the results of such a request should be a direct modification of the objects in question. RESTful boundaries aside, here is a version of the UserController method in all it's gory rails detail, with comments indicating the business logic:

  def update
    user = User.find_by_id params[:id] # with before filter - given I am signed in as this user or I am an admin
    user.update_attributes(params)  # my request should be persisted (and email sent)
    org = Organisation.find(params[:pending_organisation_id])
    if(org and not admin?)
      flash[:notice] = "You have requested admin status for #{org.name}" # I should see an alert
      redirect_to organisation_path(org.id) 
    elsif(!org and admin?)
      user.promote_to_org_admin
      flash[:notice] = "You have approved #{user.email}."
      redirect_to(users_report_path)
    else
      redirect_to :status => 404
    end

  end

I believe the particular advantage of using the update_attributes method here is that the single 'update' method could be used to support general updates of the underlying user object; although perhaps that is inviting too much complexity. Clearly this method is slightly longer than we might like, and it is tightly coupled to the rails framework, but we ARE working in the rails framework, and the risk of concealing the details of the framework behind business services is that how things are getting done is obscured. It would be fantastic to be able to specify the business logic of the system independently of the framework and then have everything just work, but the reality is that at the moment our project has no resources to move beyond Rails and there is no demand from the customer to move beyond Rails. So for the purpose of maintainability I am tempted to ditch the service and move back to this update method, which if it does not make the business logic completely transparent, at least makes the rails machinery involved obvious.

The more that I pour over these issues; and note that we have 6 or 7 related Cucumber scenarios to deal with the situation where the user requested org admin rights is not logged in, or not registered etc., the more I agonise over the repetition of sets of steps in the Cucumber feature file - I want the consequences of the request to be clear, but not necessarily have it specified clearly 6 or 7 times over; as I was saying, the more I pour over these issues, the more I think about some sort of permissions matrix relating users to the various system objects. The high level story might be in terms of users requesting access and other users approving them, but at the system level the abstraction ultimately is in terms of who can use which CRUD methods on which objects. So the desired business logic comes from a user object permissions matrix. As I continue to reflect I think I would prefer that this is what the system made explicit. The user stories should be and are clearly stated in the Cucumber features. The system should make it clear what machinery it is using to achieve the desired effects, and if the user object permissions matrix was exposed one could tweak that to achieve many of the desired outcomes.

I'm reminded of the object-role-permission systems from my J2EE days, which are rather heavy firepower for the purposes of prototyping anything quickly. The closest thing I know about in the Ruby/Rails world is the CanCan gem, which I need to investigate further, but what I really want is a nice admin HTML interface that allows me to tweak outcomes that will be persisted on the fly, rather than hard coding them in Ruby (as CanCan does?), since I think the changing nature of the clients desired outcomes will likely be supported by tweaks to that matrix. The code might look something like this:

def update
    user = User.find_by_id params[:id] # with before filter - given I am signed in as this user or I am an admin
    flash[:notice] = matrix.assemble_notice user.update_attributes(params)  # my request should be persisted (and email sent)
    redirect_to = matrix.check(params[:redirect_to])
end

So in summary I am currently on the fence about where to draw the lines of abstraction between business logic and interface/application specifics. In the first instance I'm planning to refactor away the service for the moment, check out the best practice for nested routes (which are starting to feel necessary), and keep thinking about how we can easily expose the permissions matrix to make life easiest for client, developers and users; however I'm very interested in people's opinions on everything above including Cucumber versus RSpec, Services, Permissions and code readability/maintainability; comments welcome!

Do also see the follow up to this article in part 2!

comments powered by Disqus