Refactoring membership status management to use the Observer Pattern PR 445 write up

This is a write-up for PR #445

We've learned a lot about the domain and have helped SHF think about and refine business rules since we started implementing all of this. Implementing payments have particularly made the models and underlying assumptions more complex. It's time to take what we've learned and think about how to refactor so that we are doing good, SOLID OO design.

The main function of the system is to manage membership status.

It must make sure that the membership status for every User in the system is always correct, based on the business rules for SHF.

---

Just to review some basics and provide some background to anyone not familiar with the terms and concepts:

SOLID OO Design Principles

• Single Responsibility Each class should have a single responsbility. coding strategy
• Open/Closed principle "Open" to extension (changing business requirements) but "closed" to modification (requiring lots of modifications to make that happen) a goal
• Liskov substitution Objects can be replaced with subclasses without affectingn the correctness of a program. abstract formula about software
• Interface segregation Many different interfaces are better than a few large ones (break things down into small things)
• Dependency inversion Be dependent on abstractions, not concretions (depend on the larger contract/responsibility, not the actual classes that might implement it) coding strategy, especially important for static languages (not Ruby)

These are all strategies you can apply so that when change happens (and it will), you can reduce all the dependencies and entaglements within your code.

Another way to express these are with the more classic Software Engineering principles:

Software Engineering principles

• loose coupling things are not dependent on each other
• high cohesion the purpose of a component (class) should be focused and cohesive; it should have one clear, focused responsibility
• context independent objects are complete and independent without having to depend on lots of other objects being in specific states
• easily composable many, small components can be easily put together to accompish goals without having to change code

Robert Martin: SOLID, Agile Manifesto, 'clean' coding and architecture

Wikipedia: SOLID object oriented design

Sandi Metz: SOLID Object-Oriented Design presentation at GORUCO 2009:
An excellent introductory video on SOLID principles and how to put them into practice.

This is another important, Rails specific design principle:

Skinny Controllers

• Controllers should be responsbile displaying the right views based on models and actions, not business logic

Reference: Advi Grimm: Slim down hefty Rails controllers AND models, using domain model events https://www.rubytapas.com/2017/04/11/skinny-controller-domain-model-events/

---

Current state

Currently, the responsbility for knowing if the requirements for membership have been met (should membership be granted?) is spread out between multiple classes: PaymentsController and User. This also means that the PaymentsController is responsible for an important piece of business domain logic. The controller should not be responsible for this; it should not have to know or care about who should or should not be granted membership or how granting membership is done, or what has to happen when/if membership is granted (or revoked, for that matter).

Here's a sequence diagram that shows how we currently determine if a membership should be granted and how the membership is granted. This incorporates the changes in PR 441: Do not grant membership for branding payment that moves the logic into the success method of the PaymentsController.

Note that both the PaymentsController and User are responsible for granting membership. If the business rules change, both the PaymentsController class and the User class must be modified.

PaymentsController has to interact with many different domain objects. It is highly coupled to many different classes.
That means there are many dependencies (entanglements). It should not be responsible for implementing business logic.

Additionally, the User class has many, many responsibilities. Among them, the User class:

• must determine the next membership number that should be used in the system
• must calculate the end of the current membership term, which depends on knowing the specific type of payment
• must know that mail should be sent out when membership is granted, and which email to send
• if a membership term has expired or not, which depends on knowing the specific type of payment
• if a membership payment can be made
• when the next membership payment is due, which depends on knowing about the type of payment that a 'membership fee' is

The User class clearly does not have a single responsbility. We need to refactor it and put those responsibilities elsewhere. A User class should respond and perhaps change its internal state based on essages it receives from other things in the system.

Refactoring to be more SOLID

We can be much more SOLID by setting up one class that is responsible for knowing if membership requirements have been satisfied, and another class that is responsible for granting membership. (= Single Responsibility Principle)

It's easy to reduce coupling (dependencies/entanglements)by having a class observe the state of domain objects and see if and when membership needs to be checked and possibly updated. Other classes can go on about their own business (responsibilities), secure in the knowledge that if membership needs to be updated, another class is watching out for that and will do whatever needs to be done. In other words, other classes do not share that responsibility (they should be focused on their own single responsbility). Nor are they invovled (entangled, coupled, or required depdendencies) in figuring out membership requirements or what has to happen when membership is granted.

The Observer pattern is a fundamental and common pattern in software and is easy to implement in Ruby with the standard library Observable module.

Putting it together, here is what I did when I refactored:

• created a class that has the single responsibility of knowing if a User meets the all of the requirements for being a member: RequirementsForMembership
• created a class that has the single responsibility of doing what needs to be done when membership is granted: MembershipStatusUpdater (In my code, I have also included the responsibility for revoking membership. It could easily be argued that this should be in a separate class.)
• removed those responsibilities from the User and PaymentsController classes
• used the Observer pattern to reduce dependencies (coupling) between those classes when membership is granted.

Here's the sequence diagram with the refactored code:

The sequence diagram makes it clear that the PaymentsControler now does much less. When the payment has been made successfully, the PaymentsControler now just makes sure that the payment sends the successfully_made message. The PaymentsControler does not need to know (is not coupled with or entangled with) what that 'means' or what other objects/classes that might involve.

The sequence diagram also helps to show that the process for updating membershipe (checking to see if membership requirements are being met and grantin membership) is much more encapsulated. Far fewer dependencies are invovled. ( = lower coupling)

As should happen with a refactoring, all tests that are not unit tests should still pass without being altered. For this project, that means that all cucumber tests should still pass without any changes to the .feature files. (They do pass.)

The only change that was needed to the cucumber files was to alter one step definition: And(/^I complete the payment$/) do The lines that explicitly forced that user to grant membership were removed (commented out). The membership status is now updated automatically ('naturally') by the system via the observer pattern. Since step definitions stand in for a certain amount of implementation, it is reasonable that they might have to be changed when code is refactored. In this case, the change to the step clearly reflects that a User no longer has the responsibility for granting membership.

Refactoring Responsibilities into Updaters and Requirement Checkers

There is a general pattern for what happens when we need to check to see if requirements are met and to act (or not) if they are. Here's the general sequence to see if membership status can be granted or if it needs to be revoked:

If the requirements are met for granting membership
  then do whatever needs to be done to update things so membership is granted
  else
    (since the requirements are no longer met, do we need to 'revoke' membership?)
    revoke membership if the requirements are specifically met for _revoking membership_

This is also the general pattern for checking company visibility:

If the requirements are met for making a company visible
  then do whatever needs to be done to make a company visible
  else
   make the company invisible if the requirements are met making the company invisible

Requirements Checkers

Each class that is responsible for checking to see if a requirement is met (a "Requirements Checker") just needs to answer "true" or "false" when they are asked if a requirement is satisfied? and given a Hash of information to work with.

The class does 2 things privately in order to answer .satisfied?:

1. checks to see if the Hash passed as an argument provides all of the information it needs (has_expected_keys?), and if it does, then
2. checks to see if the requirements are met (requirements_met?)

These Requirements classes encapsulate the business requirements in one place. When business requirements change, no other classes need to be modified. This follows the "Open/Closed" SOLID principle and good encapsulation.

Updaters

Generalizing the pattern what happens if requirements are met?, I created the AbstractUpdater class.

An Updater:

1. will respond to check_requirements_and_act({}) It will see if requirements have been met and then do whatever needs to be done if they are. It knows how to both "update" and to "revoke". I decide to use the terms "update" to represent the general action for "updating the item," as in to grant membership, or to make a company visible; and to use the term "revoke" to mean the 'opposite' of "update," as in to revoke membership, or make a company invisible

2. It knows which Requirements class to ask to see if the "update?" requirements have been met (using satisfied? method)

3. It knows which class to Requirements class to ask to see if the "revoke?" requirements have been met (using the satisfied? method)

The MembershipStatusUpdater uses the RequirementsForMembership class to see if membership should be updated (granted). It uses the RequirementsForRevokingMembership class to see if membership should be revoked. (These are set in the self.update_requirements_checker and self.revoke_requirements_checker methods respectively.)

If the requirements are met for updating (granting) membership, the private update_action(args) is called. For the MembershipStatusUpdater, this means updating the user so that membership is 'true' and sending out an email. For some other class, like a CompanyVisibilityUpdater, the private update_action(args) method would do whatever needed to be done so that a company is displayed to the public on the site

If instead membership should be revoked and the requirements for doing so are met, the MembershipStatusUpdater calls the private revoke_update_action(args = {}) method; the user is set so that it no longer is a member (member = false). Likewise for some other class, like a CompanyVisibilityUpdater, the private revoke_update_action(args) method would do whatever needed to be done so that a company would no longer be publicly visible on the site.

---

Here is a UML diagram that shows the relationships between Updater classes and RequirementCheckers:

(It's not a very pretty diagram. I let 'plantUML' automatically generate it and it's kind of ugly. But it means it's easy for the diagram to later be modified.)

---

Disclaimers and notes about the code:

It took me longer to write this up than it did to write the code, which is a testement to the power of the Observer pattern.

• There is surely clean up and further refactoring that can be done. I did write specifications and tests, but did not double-check to ensure that I have covered all possibilities or needed scenarios.
• I used Singletons for the Updaters, but that's not necessary; using class methods is also a valid approach.
• I orginially started recording my thoughts in the doc comment for the MembershipStatusUpdater class; I didn't clean that up.
• I put the Updater and MembershipRequirements classes under the app/model directory. They could certainly be services and go into that directory. I just started with them there out of habit and didn't want to spend too much time on that, knowing this is an exploratory spike.
• I commented out some specs instead of removing them when I refactored to (hopefully) make things clear and to show that when a specification is big, that's a smell that the class has too many responsibilities.
• I did some 'clean up' with the Payments state: I used constants to help hide the implementation (the mapping to the Hips values)