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
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.
Long complicated methods, and groups of methods on a single model.
Service Objects Example for Micro Blog
Minors are not allowed to post profanity.
User model counts the number of profanity words used by a minor.
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
After adding MicropostCreationService:
The code is much easier to test.
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.
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
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
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.
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.
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
Create a PORO in a services directory (or optionally in models).
If using the services directory, be sure to add it to the load path
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.
@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.
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.
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