Method Object Refactoring for Rails Apps and Narrow Controllers

Justin_Gordon · April 10, 2014, 9:00pm

I’m experimenting with creating a narrow controller rather than a “service object” and I’d like to get some opinions on these patterns. A more fundamental question I have is on the usefulness of creating very small methods and classes per the “Sandi Rules” if that means breaking classes up more on a “technical” level as opposed to a “domain” level. The things I like about very small methods and classes is that:

Limited file size makes it easy to see the private methods behind a public method.
Instance variables have a narrowly defined purpose.
Since tests correspond typically to a class, as small class will have a proportionally small test file, which is also easier to grok.

How do we reconcile the difference between:

“technical” refactoring to create a class that conforms to the Sandi rules.
Refactoring based on the problem domain.

Could we possibly use a module grouping to capture the domain relationship between two method objects?

Here’s my two tries at refactoring an example of a complicated controller method. What do you like or not like about each?

github.com/shakacode/fat-code-refactoring-techniques

Earlier Draft: Single purpose controller

shakacode:fat_controller_method_setup ← shakacode:single_purpose_controller

opened 09:42AM - 09 Apr 14 UTC

justin808

+205 -51

# Single Purpose Controller This is a preferable alternative to pull request ht…tps://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. [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/justin808/fat-code-refactoring-techniques/7)

github.com/shakacode/fat-code-refactoring-techniques

Earlier Draft: Service objects

shakacode:presenters ← shakacode:service_objects

opened 07:49AM - 06 Apr 14 UTC

justin808

+313 -16

# 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 https://github.com/justin808/fat-code-refactoring-techniques/pull/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. 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: ``` ruby 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. ``` ruby 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: ``` 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. 3. 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 ``` 4. 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 ``` 5. 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. 6. 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. 7. 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 ``` ruby 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. [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/justin808/fat-code-refactoring-techniques/6)

Single Purpose Controller

This is a preferable alternative to pull request Earlier Draft: Service objects by justin808 · Pull Request #6 · shakacode/fat-code-refactoring-techniques · GitHub, 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.

Here is the code example:

class MicropostCreateController < ApplicationController
  before_action :signed_in_user

  # By having one public method, all private methods relate to this one method
  # and any instance variables can apply to all private methods
  def create
    @micropost = Micropost.new(micropost_params.merge(user: current_user))
    return if current_user.minor? && used_profanity
    save_micropost
  end

  private

    def save_micropost
      if @micropost.save
        flash[:success] = "Micropost created!"
        redirect_to root_url
      else
        error_rendering
      end
    end


    # return true if checked to ensure no profanity violations
    def used_profanity
      if profanity_words.present?
        profanity_found_side_effects
        profanity_found_controller_response
      end
    end

    def profanity_words
      @profanity_words ||= ProfanityChecker.new(@micropost.content).profanity_words_contained
    end

    def profanity_found_controller_response
      flash.now[:error] = <<-MSG
            Whoa, better watch your language! Profanity: '#{profanity_words.join(", ")}' not allowed!
            You've tried to use profanity #{view_context.pluralize(current_user.profanity_count, "time")}!
      MSG
      error_rendering
      true
    end

    def error_rendering
      @feed_items = []
      render 'static_pages/home'
    end

    def profanity_found_side_effects
      current_user.increment(:profanity_count, profanity_words.size)
      current_user.save(validate: false)
      send_parent_notifcation_of_profanity
    end

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

    def micropost_params
      params.require(:micropost).permit(:content)
    end
end

And here is the corresponding test:

require 'spec_helper'
describe MicropostCreateController do
  let(:minor) { FactoryGirl.create :user, minor: true, profanity_count: 0 }
  let(:adult) { FactoryGirl.create :user, minor: false, profanity_count: 0 }
  let(:profanity_content) { "Dad is a poopface and fartface!" }
  let(:clean_content) { "Dad is the best!" }

  def flash_now
    flash.instance_variable_get(:@now)
  end

  context "does contain two word profanity" do
    subject(:service) { MicropostCreationService.new(minor, profanity_content) }
    context "a minor" do
      before { sign_in minor, no_capybara: true }

      it "gives a flash with poopface and fartface" do
        post :create, micropost: { content: profanity_content }

        # Demonstrate testing flash.now
        expect(flash_now[:error]).to match /poopface/
        expect(flash_now[:error]).to match /fartface/
        expect(flash_now[:error]).to match /2 times/

        # Demonstrate checking rendered template
        expect(response).to render_template('static_pages/home')

        # Demonstrate checking instance variable
        expect(assigns(:feed_items)).to eq([])

        expect(response.status).to eq(200)
      end

      it "increases the minor's profanity count by 2" do
        expect do
          post :create, micropost: { content: profanity_content }
        end.to change { minor.reload.profanity_count }.by(2)
      end

      it "does not increase the number of posts" do
        expect do
          post :create, micropost: { content: profanity_content }
        end.not_to change { Micropost.count }
      end
    end

    context "adult" do
      before { sign_in adult, no_capybara: true }

      it "gives a flash with poopface and fartface" do
        post :create, micropost: { content: profanity_content }
        expect(flash[:success]).to eq("Micropost created!")
        expect(response).to redirect_to(root_url)
      end

      it "does not change the profanity count" do
        expect do
          post :create, micropost: { content: profanity_content }
        end.not_to change { adult.reload.profanity_count }
      end

      it "does increase the number of posts" do
        expect do
          post :create, micropost: { content: profanity_content }
        end.to change { Micropost.count }.by(1)
      end
    end
  end
  context "no profanity" do
    context "minor" do
      before { sign_in minor, no_capybara: true }

      it "gives a flash with poopface and fartface" do
        post :create, micropost: { content: clean_content }
        expect(flash[:success]).to eq("Micropost created!")
        expect(response).to redirect_to(root_url)
      end

      it "does not change the profanity count" do
        expect do
          post :create, micropost: { content: clean_content }
        end.not_to change { minor.reload.profanity_count }
      end

      it "does increase the number of posts" do
        expect do
          post :create, micropost: { content: clean_content }
        end.to change { Micropost.count }.by(1)
      end
    end
  end
end

Topic		Replies	Views
Service Objects Refactoring Example Ruby on Rails	4	3299	April 11, 2014
Refactoring Hartl's Rails Tutorial using Concerns Ruby on Rails	6	1373	April 7, 2014
Comments about a design pattern code at the Rails Conf Ruby on Rails	1	609	May 27, 2014
Refactoring fat models, fat controllers, fat views --> Options Ruby on Rails	8	3375	March 24, 2014
Refactoring Question Ruby on Rails	1	656	November 12, 2014

Method Object Refactoring for Rails Apps and Narrow Controllers

Single Purpose Controller

Related topics