← Back to Upcase

Service Objects Refactoring Example

(Justin Gordon) #1

I created this refactoring example for my RailsConf 2014 talk on refactoring fat controllers.

Here is the code for this example:

Please feel free to make comments in the PR or in this message thread. Thanks in advance for any feedback on this topic. I think the example is pretty fun. I add profanity checking for minors to Michael Hartl’s microblog post application. Naturally, I was inspired by my 5 year old son.

Service Objects

This is a variation of the Presenter technique, with an emphasis on complex
model interactions. A Service Object called by a controller should return an
object that encapsulates what the controller should do. This includes flash
messages, redirects, etc. The example class ControllerResponse provides a
sample of how to do this. A service object could create a presenter for the
controller to pass to the view.

Service Objects Applicable Situation

  1. Some action involving the interaction between several models, such as:
    • A customer creating an order and charging the purchase.
    • Post login process for a user.
  2. Long complicated methods, and groups of methods on a single model.

Service Objects Example for Micro Blog

  1. Minors are not allowed to post profanity.

  2. User model counts the number of profanity words used by a minor.

  3. First commit presents a tangled up controller method:

        def create
          @micropost = current_user.microposts.build(micropost_params)
          ok = true
          if current_user.minor?
            profanity_word =  profanity_word(@micropost.content)
            if profanity_word.present?
              flash.now[:error] = "You cannot create a micropost with profanity: '#{profanity_word}'!"
              current_user.increment(:profanity_count)
              @feed_items = []
              render 'static_pages/home'
              ok = false
            end
          end
          if ok
            if @micropost.save
              flash[:success] = "Micropost created!"
              redirect_to root_url
            else
              @feed_items = []
              render 'static_pages/home'
            end
          end
        end
    

    This code is not easily testable, being on the controller. The addition of the
    profanity_words method on the controller also does not fit. I don’t go into
    building tests, as the refactor will allow easy testing.

        PROFANITY_WORDS = %w(poop fart fartface poopface poopbuttface)
        
        # Yes, this could go into a validator for the micropost, but let's suppose there's reasons
        # that we don't want to do that, such as we only want to filter profanity for posts
        # created by minors, etc.
        # returns profanity word if existing in content, or else nil
        def profanity_word(content)
          words = content.split(/\W/)
          PROFANITY_WORDS.each do |test_word|
            puts "test_word is #{test_word}, words is #{words}"
            return test_word if words.include?(test_word)
          end
          nil
        end
    
  4. After adding MicropostCreationService:

    1. The code is much easier to test.
    2. The controller method is much simpler:
           def create
             response = MicropostCreationService.new(current_user, micropost_params[:content]).create_micropost
             response.apply(self)
           
             unless response.ok # if not ok, then we did a redirect when calling response.apply
               @micropost = response.data[:micropost]
               @feed_items = []
               render 'static_pages/home'
             end
           end 

The response.apply method does things like setting the flash message and
the response code.

  1. The main method of MicropostCreationService is simple:
    def create_micropost
      micropost = @user.microposts.build(content: @content)
      response  = check_profanity(micropost)
      unless response
        response = save_micropost(micropost)
      end
      response
    end
  1. Note how the Service utilizes the ControllerResponse class so as not be
    throwing exceptions for error scenarios.
  def save_micropost(micropost)
    if micropost.save
      ControllerResponse.new(flash_message: "Micropost created!",
                             flash_type:    :success,
                             redirect_path: h.root_url)
    else
      ControllerResponse.new(http_status: :bad_request, data: { micropost: micropost })
    end
  end

  # return response or
  def check_profanity(micropost)
    if profanity_word
      msg = "Whoa, better watch your language! Profanity: '#{profanity_word}' not allowed!"
      @user.increment(:profanity_count)
      ControllerResponse.new(flash_message: msg,
                             flash_type:    :error,
                             flash_now:     true, # True b/c not redirecting
                             http_status:   :bad_request,
                             data:          { micropost: micropost })
    end
  end
  1. A good test of code is how it changes when requirements change. Suppose that
    the code should print all the profanity in the message, and add the number of
    profanity words to the user’s profanity counter.
  2. Putting the profanity calculation into it’s own class further shrinks the
    size of classes and clarifies the tests. The main advantage of having the
    ProfanityChecker into its own class is that the Micropost can also use the
    logic for validation.
  3. A slight error was left in the commits along with the fix. This is to show
    the usefulness of having concise and simple unit tests on the
    MicropostCreationService.

Service Object Solution

  1. Create a PORO in a services directory (or optionally in models).
  2. If using the services directory, be sure to add it to the load path
        config.autoload_paths += %W(
          #{config.root}/app/services
        )
  1. Move all applicable methods into this class from the model.
  2. Use an instance of ControllerResponse to return the responses back to the
    controller.
  3. Create test in spec/services directory.

Service Object Advantages

  1. Clearly bundles code around a specific, complex action.
  2. Promotes the creation of smaller, tighter methods when all methods in a small
    class are around a single purpose.
  3. Allows better unit testing of methods on this service object.

Service Object Disadvantages

  • It can be overkill for a very simple action.
  • Something simply and generally useful on the model is better put in a concern
    or decorator, or maybe a method class called by the model.
2 Likes

(rubylove.io) #2

Wow @Justin_Gordon, that was quite an effort! Excellent material.

I think I am in the minority camp. I definitely prefer ‘service objects’ to the highly coupled decorator or worse, the concern, however I still prefer to move into the /lib folder and start creating libraries that will be extracted into gems.

Excellent work, it definitely cleaned up that method.

0 Likes

(Luís Ferreira) #3

@Justin_Gordon I like service objects and use them sparingly, I don’t like the fact that your service object is setting flash messages and status codes. That means that the service knows it belongs to a web app and thus you have dependencies both to and from the service.

I try to make my dependency graph point always in the same direction. Also, it might be argued that doing those things is part of the job description of the controller. When adding a service layer it should be there to perform actions and report back, the controller should always be the one responsible for parsing input and constructing the output, in my opinion.

Here’s a nice talk on a variation of this by Josh Cheek of 8th light. He calls them function objects.

0 Likes

(Justin Gordon) #4

I’ve created an alternative solution! This is probably better and probably suits most cases of where one believes they need a “service object”.

https://github.com/justin808/fat-code-refactoring-techniques/pull/7

Single Purpose Controller

This is a preferable alternative to pull request https://github.com/justin808/fat-code-refactoring-techniques/pull/6, which uses a Service Object pattern.

Objectives:

  • Splitting the MicropostController into the MicropostCreateController to so
    that a complicated controller method can be refactored to have many private
    methods that relate to the only public method, on the “single purpose
    controller”. I.e. conform to the Sandi rules of small methods.

  • Demonstrate changing the routing for a custom named controller.

  • Demonstrate a controller spec that covers everything the service object spec
    covered, but covers it more accurately because it actually verifies that the
    controller is doing the right thing.

  • Demonstrate following features of controller spec testing in rspec:

    • testing flash.now
    • checking instance variable
    • checking rendered template
  • Demonstrate using a model object, ProfanityChecker to handle logic for a
    potentially complex validation.

  • Demonstrates simple tests of this ProfanityChecker class.

0 Likes

(Justin Gordon) #5

I incorporated feedback from the previous two fat controller refactoring attempts into:

https://github.com/justin808/fat-code-refactoring-techniques/pull/8

Here’s the takeaways:

  • Use Rails constructs first. Don’t invent patterns that don’t need to be invented.
  • Think in terms of domains, not patterns or other “technical” refactorings. At the same time, strive for:
    • Small methods
    • Small classes
    • Clarity
    • Simplicity
  • The key problem is putting code in the right place:
    • Models: business logic goes here, and doesn’t have to be an AR model.
    • Controller: Interaction code goes here. Try to move any business logic into the Model layer.
    • Presenter: Encapsulate preparation of objects for view rather than setting up many instance variables in the controller (see http://forum.thoughtbot.com/t/presenters-refactoring-example/2324).
  • Have a good toolbox for good domain based Refactoring including:
    • Concerns, for both model and controller, which are better than includes when callbacks and static methods are involved.
    • Don’t be afraid to create non AR based models.
    • Presenter pattern.

Here’s the key bits of code. The tests are focused and clear, and can be found in the github repo.

Thanks in advance to those that contributed this discussion. That has helped immensely in the preparation of my RailsConf talk.

Any comments, criticisms, advice, etc. are totally appreciated!

Thanks,

Justin

MicropostsController

class MicropostsController < ApplicationController
  before_action :signed_in_user, only: [:create, :destroy]
  before_action :correct_user,   only: :destroy

  def create
    @micropost = Micropost.new(micropost_params.merge(user: current_user))
    if @micropost.save_checking_profanity
      flash[:success] = "Micropost created!"
      redirect_to root_url
    else
      set_flash_for_profanities
      render 'static_pages/home'
    end
  end

  def destroy
    @micropost.destroy
    redirect_to root_url
  end

  private
    # Example of customized flash message when the standard validation is not sufficient
    def set_flash_for_profanities
      if @micropost.profanity_words_for_minor.present?
        flash.now[:error] = <<-MSG
                Whoa, better watch your language! Profanity: '#{@micropost.profanity_words_for_minor.join(", ")}' not allowed!
                You've tried to use profanity #{view_context.pluralize(current_user.profanity_count, "time")}!
        MSG
      end
    end

    def correct_user
      @micropost = current_user.microposts.find_by(id: params[:id])
      redirect_to root_url if @micropost.nil?
    end
end

Micropost

class Micropost < ActiveRecord::Base
  belongs_to :user
  validate :profanity_checker

# Unrelated code omitted for clarity

  # Will check for profanities if a user a minor, and if content OK, then save
  def save_checking_profanity
    if profanity_words_for_minor.present?
      user.minor_tried_to_use_profanities(profanity_words_for_minor)
      return false
    end
    save
  end

  def content=(content)
    @profanity_words_for_minor = nil
    self[:content] = content
  end

  # profanities only counted if user is a minor
  def profanity_words_for_minor
    @profanity_words_for_minor ||= if user.minor?
                           ProfanityChecker.new(content).profanity_words_contained
                         else
                           []
                         end
  end

  private
  def profanity_checker
    return unless user && user.minor?

    profanity_words = ProfanityChecker.new(content).profanity_words_contained
    errors.add(:content, "Profanity: '#{profanity_words.join(", ")}' not allowed!") if profanity_words.present?
  end
end

User

class User < ActiveRecord::Base
  # CODE OMITTED FOR CLARITY

  # Actions to take if minor tries to use profanities
  # profanities List of words attempted to use
  def minor_tried_to_use_profanities(profanity_words)
    raise "Called minor_tried_to_use_profanities with adult" unless minor?
    increment(:profanity_count, profanity_words.size)
    save(validate: false)
    send_parent_notifcation_of_profanity
  end

  private
    def send_parent_notifcation_of_profanity
      # pretend we sent an email, using @profanity_words in the content
    end
end

ProfanityChecker

class ProfanityChecker
  PROFANITY_WORDS = %w(poop fart fartface poopface poopbuttface)

  def initialize(content)
    @content = content
  end

  # returns list of profanity words (empty list if none)
  def profanity_words_contained
    profanity_list = []
    words = @content.split(/\W/)
    PROFANITY_WORDS.each do |test_word|
      profanity_list << test_word if words.include?(test_word)
    end
    profanity_list
  end
end
0 Likes