1
\$\begingroup\$

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

module Enrolments
  class Suspend
    attr_reader :enrolment, :responses, :comment, :user_id
    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

    attr_reader :response

    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.

\$\endgroup\$
0
1
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ 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 \$\endgroup\$ Aug 5 '15 at 6:09
0
\$\begingroup\$

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_reader :enrolment, :comment
    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

    attr_reader :responses

    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_reader :enrolment, :responses, :comment, :user_id
    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

    attr_reader :response

    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
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.