1

A user has to perform a somewhat complex task on my website (submitting a rating). For the sake of example, let's consider it's a rating for a movie, done through a controller. There are several possible "paths" the logic could follow

  • The user wants to cancel his rating submission (eg he didn't go to the movie)
    • He provides all the required parameters OR not
  • The user submits the rating submission
    • Providing all the required parameters OR not
  • The user had already submitted his rating ( and is denied the action)
  • Other reasons (eg movie deleted, etc.)

I am trying to encapsulate all the logic in a Service/Mediator, so I can also reuse the logic outside a controller (eg. in an operational console).

I decided to make the controller an argument during this RatingService instanciation, and this is turning into an inversion of control/the service does everything. My service knows what state it is currently in, and triggers various callbacks (notification, tracking, etc.) including rendering "flashes" for the user through the controller.

This starts smelling weird since my service is slowly taking possession of the controller for itself.

I am showing some sample code (Ruby on Rails) just to illustrate my point

class ReviewController
  def review
     ReviewService.review(reviewable, current_user, 
                          controller: self, **review_params)
  end
end

class ReviewService
  def review
     ...
  end

  private

  def cancelled_submission?
    ...
    controller.flash(I18n.t(:cancelled)) if controller.present?
  end

  def submit_rating
    ...
    controller.flash(I18n.t(:rating_submitted)) if controller.present?
  end

  def already_submitted
    ...
    controller.flash(I18n.t(:already_submitted)) if controller.present?
  end
end

I am wondering if it would be a better practice to consider a full inversion of control, and only declare methods in my controller that can be used in the service.

That is, turn

class ReviewController
  def review
     ...
  end
end

class ReviewService
  def review
     ...
  end

  private

  def cancelled_submission?
    ...
    controller.flash(I18n.t(:cancelled)) if controller.present?
  end
end

Into

class ReviewController
  def review
     ...
  end

  # Public method, meant to be called back from the service in an inversion of control fashion
  def notify_cancelled
    flash(I18n.t(:cancelled)
  end
end

class ReviewService
  def review
     ...
  end

  private

  def cancelled_submission?
    ...
    controller.notify_cancelled if controller.present?
  end
end

..or if there are better practices out there

  • Controllers are supposed to be "thin" anyway. If you feel like your mediator is turning into a god class, then break it into separate classes. – Robert Harvey Apr 7 '17 at 19:19

Your Answer

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

Browse other questions tagged or ask your own question.