Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Earlier Draft: Service objects #6

Open
wants to merge 8 commits into
base: presenters
Choose a base branch
from
Open

Conversation

justin808
Copy link
Member

Service Objects

This code demonstrates moving complex controller code into a class that manages model interactions, aka a Service Object. The controller calls the Service Object and it returns 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.

After getting this reviewed, I've developed an alternative approach that focuses on keeping controller parts (setting flash, etc.) in the controller and moves other logic into the model code. See #12.

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.
  1. 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
  1. After adding MicropostCreationService:
  2. The code is much easier to test.
  3. The controller method is much simpler:
  ``` ruby
      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:
  ``` ruby
        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.
  ``` ruby
      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.

Review on Reviewable

* Display if minor on profile page.
* Add creation of minor to sample_data.rake
* Too much logic in controller method create.
* No tests added as not very convenient for a complicated controller
  method!
* Subtle bug in this commit
* Handles checking profanity for minors, and adds profanity_count on
  users table.
* Sample data creates user 'littlepunk@sugarranchmaui.com' who is a
  minor.
* Added tests on service object.
* Clarified and simplified tests.
* Left in 2 broken tests regarding the profanity counter.
* All tests now pass
* However, if you try this with a minor:
  * You get the correct error message.
  * BUT, the profanity is posted!
* Also added test confirming post created for non-profanity.
* Don't use @user.microposts.build(content: @content) because we will
  save the user when there's profanity.
* Instead, call Micropost.new
* Fixes broken test
* Example of using the ProfanityChecker service in multiple places.
@jodosha
Copy link

jodosha commented Apr 7, 2014

This service is too much coupled with the knowledge of a Rails controller (eg. flash_message, flash_type). A service should perform business logic independently from the delivery method (a controller, in this case). It should return enough informations and let the caller to take the right decision.

I'd like to share some nice to have things:

  1. Naming things after patterns shouldn't be preferable. Also, that service is an use case, what do you think of a name like CreateMicropost?
  2. Use a consistent public API across your services. Instead of #create_micropost, #edit_micropost, use something like, #call, #run, #execute and stick with that name.
  3. That profanity check isn't a validation, but a policy. A validation is about to the presence of an attribute, if you expect a number but receive a string, etc.. A policy is something related to the business rules of your domain: we don't want profanities in our system. I'm experimenting with policies and planning to blog about them soon.

@gylaz
Copy link

gylaz commented Apr 8, 2014

Service Object is not a variation of a Presenter technique. They are two different things. Presenter helps out with the presentation layer, while Service layer acts as an intermediary between multiple models (and doesn't care about presentation per se). This article from Martin Fowler should shine some more light on the Service layer.

I agree with @jodosha that the pattern you are presenting is too coupled to the controller. Perhaps rename it to "Response Object", because I think there is something interesting here, but I would not confuse it with Service Objects.

@justin808 justin808 changed the title Service objects Earlier Draft: Service objects Apr 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants