# Managing course enrollment

I have written some code to suspend someone's enrollment to a course. A course may have multiple presentations.

module Enrolments
class Suspend
attr_writer :event_service, :suspend_enrolment_presentation_service

def initialize
@responses = []
end

def call(enrolment, user_id, comment)
@enrolment = enrolment
@user_id = user_id
@comment = comment

suspend_enrolment_presentations
if all_enrolment_presentations_suspended?
suspend_enrolment
raise_suspended_enrolment_event
create_enrolment_comment
end
end

private

def suspend_enrolment_presentations
enrolment.enrolment_presentations.each do |ep|
@responses << suspend_enrolment_presentation_service.call(ep)
end
end

def all_enrolment_presentations_suspended?
@responses.all?{ |r| r[:success] }
end

def suspend_enrolment
enrolment.status = 'Suspended'
enrolment.save
end

def raise_suspended_enrolment_event
event_service.call(enrolment)
end

def create_enrolment_comment
EnrolmentComment.create(comment: comment, user_id: user_id, enrolment_id: enrolment.id)
end

def event_service
@event_service ||= Events::Outgoing::Enrolments::SuspendedEnrolment.new
end

def suspend_enrolment_presentation_service
@suspend_enrolment_presentation_service ||= Enrolments::EnrolmentPresentations::Suspend.new
end
end
end


The code is fairly straight forward. It has a couple of dependencies that are injected via attr_writers when testing, otherwise they are instantiated when accessed.

I have heard/read a lot of issues regarding callbacks. The problem with this code is if I ever want to just suspend an enrollment and not raise an event or create a comment, I can't. Some ways to get around this are with the decorator pattern or I guess I could compose and object of all the services and manage everything in there.

I really strive for SRP and good design in my code. I would really appreciate any input on how I could improve this code. Should I be extracting the code to a higher level of abstraction and dealing with it one level up? What if running one piece of code depends on another passing? If I was to extract this out it would land up in my controller which is as high as I can go, unless I then extracted that into a form object.

You could get rid of the service methods, initialise the values in the initialize method. And then in a subclass set a service that doesn't actually do anything with the enrollment.

class Enrolments::Suspend
...
def initialize
@responses = []
# get rid of the methods and set the services in the intitialiser
@event_service ||= Events::Outgoing::Enrolments::SuspendedEnrolment.new
@suspend_enrolment_presentation_service ||= Enrolments::EnrolmentPresentations::Suspend.new
end
...
end

class EnrollmentWithoutEvent < Enrolments::Suspend
...
def initialize
super
# do default config here
@event_service ||= Events::DoNothing.new
end
...
end


Or, you could create a subclass that overrides the raise_suspended_enrolment_event method:

class Enrolments::SuspendWithoutErrors < Enrolments::Suspend
def raise_suspended_enrolment_event
# change the implementation here
end
end


Another options would be to use class_attribute for your service objects/procs:

class Enrolments::Suspend
...
class_attribute :event_service, :suspend_enrolment_presentation_service
...
end


Then, when you subclass, you get set your services per class or instance and can change/add the services on any sub-classes or instances of sub-classes. Have a look at here for how it works. This method means that you can create classes with specific services, and when you instantiate the class, the instances also use the same services.

For example:

class Enrolments::Suspend
...
class_attribute :event_service, :suspend_enrolment_presentation_service

# create the services used for this class
self.event_service = proc {|enrollment| ... }
self.suspend_enrolment_presentation_service = proc {|e| ... }
end

class SuspendWithoutError < Enrolments::Suspend
# override one of the services but keep the rest
self.suspend_enrolment_presentation_service = proc {|e| ... }
end


The final option is to use actual methods:

class Enrolments::Suspend
...

def event_service(enrollment)
...
end

def suspend_enrolment_presentation_service(enrollment)
...
end
end


This allows any sub-classes to override the methods.

• Thanks for the review. It definitely gave me some options. Just a couple of notes. The service methods are written in such a way to allow for easy dependency injection while testing - this could have been done in the constructor, but I prefer this method. Regarding the use of inheritance, I am not a fan of the way you have implemented it since any child classes still seem to be dependent on the actual callback. If I ended up deleting the event_service class I would need to change both the Enrolments::Suspend and SuspendWithoutError. I should only need to change one place. Thx again – Ryan-Neal Mes Aug 5 '15 at 6:09

As the original class was breaking SRP in my opinion, I decided to split up the class into two classes. One handles the coordination of things that need to happen when a suspension happens. This allows me to contain just the actual suspension logic to Enrolments::Suspend and any other required callbacks were put into the coordinator. This way each class should have only one reason to change.

module Enrolments
class SuspendCoordinator
attr_writer :event_service, :suspend_enrolment_service

def call(enrolment, comment)
@enrolment = enrolment
@comment = comment

if suspend_enrolment
raise_suspended_enrolment_event
create_enrolment_comment
end
end

private

def suspend_enrolment
suspend_enrolment_service.call(enrolment)
end

def raise_suspended_enrolment_event
event_service.call(enrolment)
end

def create_enrolment_comment
EnrolmentComment.create(comment: comment, user_id: RequestRegistry.current_user.id, enrolment_id: enrolment.id)
end

def suspend_enrolment_service
@suspend_enrolment_service ||= Enrolments::Suspend.new
end

def event_service
@event_service ||= Events::Outgoing::Enrolments::SuspendedEnrolment.new
end
end
end

module Enrolments
class Suspend
attr_writer :suspend_enrolment_presentation_service

def initialize
@responses = []
end

def call(enrolment)
@enrolment = enrolment

suspend_enrolment_presentations
suspend_enrolment if all_enrolment_presentations_suspended?
end

private

def suspend_enrolment_presentations
enrolment.enrolment_presentations.each do |ep|
@responses << suspend_enrolment_presentation_service.call(ep)
end
end

def suspend_enrolment
enrolment.status = 'Suspended'
enrolment.save
end

def all_enrolment_presentations_suspended?
@responses.all?{ |r| r[:success] }
end

def suspend_enrolment_presentation_service
@suspend_enrolment_presentation_service ||= Enrolments::EnrolmentPresentations::Suspend.new
end
end
end