Good morning,
Here is my take on the PhotoShoutsController and TextShoutsController refactor:
https://gist.github.com/curiositypaths/4d65972950c07f677a0d
Jason
This is what I did. I would love to see how others tackled this refactor. Please let us know if these are on the right track.
Here’s how I worked out the refactoring of PhotoShoutsController and TextShoutsController.
I figured that the duplicated code was all about creating a shout, so I extracted the code into a ShoutCreator class. Now my two controllers have code like so:
TextShoutsController:
def create
sc = ShoutCreator.new(current_user, TextShout,text_shout_parameters,:body)
flash.alert = sc.notice
redirect_to dashboard_path
end
PhotoShoutsController:
def create
sc = ShoutCreator.new(current_user, PhotoShout, photo_shout_parameters,:image)
flash.alert = sc.notice
redirect_to dashboard_path
end
and the new object in app/models/shout_creator.rb:
class ShoutCreator
attr_accessor :notice
extend ActiveModel::Naming
def initialize(current_user, shout_model, parameters, attribute)
@current_user = current_user
@shout_model = shout_model
@parameters = parameters
@attribute = attribute
content = build_content
shout = @current_user.shouts.build(content: content)
if shout.save then
notice = 'Shouted!'
else
notice = "Could not shout."
end
end
private
def build_content
@shout_model.__send__ :new, @parameters
end
end
I’m a bit uneasy about the “flash.alert” part… maybe my ShoutCreator method should be called ‘alert’ or most likely there’s a better way.
I’m also not happy about the location of the ShoutCreator file (maybe it should be in app/models/concerns?) and what about the extend ActiveModel::Naming… the refactoring works without it so maybe I should just drop that?
Anyway, it all works like this…
Here’s what I came up with:
class TextShoutsController < ApplicationController
def create
if !shout.save
flash.alert = "Could not shout."
end
redirect_to dashboard_path
end
private
def shout
current_user.shouts.build(content: build_content)
end
def build_content
TextShout.new(text_shout_parameters)
end
def text_shout_parameters
params.require(:text_shout).permit(:body)
end
end
and
class PhotoShoutsController < ApplicationController
def create
if !shout.save
flash.alert = "Could not shout."
end
redirect_to dashboard_path
end
private
def shout
current_user.shouts.build(content: build_content)
end
def build_content
PhotoShout.new(photo_shout_parameters)
end
def photo_shout_parameters
params.require(:photo_shout).permit(:image)
end
end
@halogenandtoast, I know there is duplication between these classes, but I’m not sure how much extraction is too much. I tried extracting the common logic into a concern, but I think I took it too far and sacrificed readability.
module Concerns
module ShoutCreating
extend ActiveSupport::Concern
def create
if !shout.save
flash.alert = "Could not shout."
end
redirect_to dashboard_path
end
private
def shout
current_user.shouts.build(content: shout_content)
end
def shout_content
raise "You must implement a shout_content method"
end
end
end
class TextShoutsController < ApplicationController
include Concerns::ShoutCreating
private
def shout_content
TextShout.new(text_shout_parameters)
end
def text_shout_parameters
params.require(:text_shout).permit(:body)
end
end
class PhotoShoutsController < ApplicationController
include Concerns::ShoutCreating
private
def shout_content
PhotoShout.new(photo_shout_parameters)
end
def photo_shout_parameters
params.require(:photo_shout).permit(:image)
end
end
Any guidance as to the appropriate level of extraction would be appreciated.
This refactor took me a little while but I eventually came up with a solution that I think is elegant. You guys can tell me what you think.
My thought process went as follows. The TextShoutsController and the PhotoShoutsController need to know the type of shout to create and the parameters to pass to the shout in order to create it. These two things are really the only things that change per shout. With that in mind I decided to create a builder class that could be used to make the appropriate type of shout.
Here is how I wished to use the builder class:
# Build a TextShout
ShoutBuilder.new(:text, current_user, params).build
# Build a PhotoShout
ShoutBuilder.new(:photo, current_user, params).build
Hence, the corresponding controllers would now look as follows:
class TextShoutsController < ApplicationController
def create
shout = ShoutBuilder.new(:text, current_user, params).build
if shout.save
redirect_to dashboard_path
else
redirect_to dashboard_path, alert: 'Could not shout.'
end
end
end
class PhotoShoutsController < ApplicationController
def create
shout = ShoutBuilder.new(:photo, current_user, params).build
if shout.save
redirect_to dashboard_path
else
redirect_to dashboard_path, alert: 'Could not shout.'
end
end
end
Notice: I chose to pass in the entire params hash. By doing it that way I could let the builder determine how to extract the shout parameters. And now the only things that are different between both controllers are the controller names and the type of shout argument. You’d see why I wanted to do it this way a little later.
Let me show you step by step how I figured out how to write the ShoutBuilder class. The constructor and the build method were fairly simple.
class ShoutBuilder
def initialize(type, user, params)
@type = type
@user = user
@params = params
end
def build
content = build_content
@user.shouts.build(content: content)
end
end
But how do we build the content with build_content? We need to use @type to determine the shout class.
class ShoutBuilder
class UnknownShoutError < StandardError; end
private
def model_class
class_name = "#{@type.to_s.capitalize}Shout"
class_name.constantize
rescue NameError
raise UnknownShoutError, class_name
end
end
So, for example, if @type = :photo then model_class returns the constant PhotoShout.
For TextShout I knew I needed to create the shout with params.require(:text_shout).permit(:body) and for PhotoShout I needed params.require(:photo_shout).permit(:image). I spent quite a while trying to determine whose concern it was to know what to do with the params hash. In the end I decided that the respective model classes should take on that responsibility (If anyone else thinks otherwise I’d really like to know why and where it should go instead). I decided that each specific shout model should have a class method called shout_params that takes one argument and returns the appropriate permitted parameters.
So adding these methods we get,
class TextShout < ActiveRecord::Base
def self.shout_params(params)
params.require(:text_shout).permit(:body)
end
end
and
class PhotoShout < ActiveRecord::Base
def self.shout_params(params)
params.require(:photo_shout).permit(:image)
end
end
So finally, the build_content method can be written as
class ShoutBuilder
private
def build_content
klass = model_class
klass.new(klass.shout_params(@params))
end
end
And here’s the ShoutBuilder in its entirety,
class ShoutBuilder
def initialize(type, user, params)
@type = type
@user = user
@params = params
end
def build
content = build_content
@user.shouts.build(content: content)
end
class UnknownShoutError < StandardError; end
private
def build_content
klass = model_class
klass.new(klass.shout_params(@params))
end
def model_class
class_name = "#{@type.to_s.capitalize}Shout"
class_name.constantize
rescue NameError
raise UnknownShoutError, class_name
end
end
After I checked that the refactoring was indeed working I decided that the controllers still had too much duplicated code. I deleted the two controllers and moved the create method into the ShoutsController.
class ShoutsController < ApplicationController
def create
shout = ShoutBuilder.new(params[:shout_type], current_user, params).build
if shout.save
redirect_to dashboard_path
else
redirect_to dashboard_path, alert: 'Could not shout.'
end
end
end
How can I set the shout type? I figured default parameters would do the trick. My routes file changed as follows:
resources :text_shouts, only: [:create], controller: 'shouts', defaults: { shout_type: :text }
resources :photo_shouts, only: [:create], controller: 'shouts', defaults: { shout_type: :photo }
@halogenandtoast: If there is anyway I could improve this code further I’d be very happy to hear.
I hope this will be valuable to someone. Happy hacking!
I’m currently working on a TDD app that has a polymorphic association structure a lot like this video series and so I’m trying out your strategy for cleaning up my controllers. While I like the overall ShoutBuilder you have, the one part I don’t like is having the PhotoShout and TextShout models being where the ::shout_params methods are.
At first I had my code like yours with a ::shout_params class method, but I was finding it hard to test. I had a mistake in the TextShout::shout_params method which was causing specs in my ShoutBuilder spec to fail. Did you do tests for these classes? Did you have to stub out that class method?
Either way, to me, the sifting of any params doesn’t belong in the model layer. It’s typically in the controller and I feel in this case it belongs in a layer somewhere in between M and C, in something like the ShoutBuilder. What if you had a TextShoutBuilder and a PhotoShoutBuilder that inherited from ShoutBuilder and each had their own #content_params method?
Here’s my solution, building off of @dwaynecrooks:
Create a ShoutBuilder model:
class ShoutBuilder
def initialize(user, params)
@user = user
@params = params
end
def build
@user.shouts.build(content: content)
end
private
def content
klass.new(shout_params)
end
def shout_params
@params.require(type).permit(klass::PERMITTED_ATTRIBUTES)
end
def klass
@_klass ||= type.to_s.camelize.constantize
end
def type
@_type ||= @params[:shout_type]
end
end
Add a line to TextShout model:
PERMITTED_ATTRIBUTES = [:body]
And to PhotoShout model:
PERMITTED_ATTRIBUTES = [:image]
Move shout creation back to ShoutsController:
class ShoutsController < ApplicationController
def show
@shout = Shout.find(params[:id])
end
def create
shout = ShoutBuilder.new(current_user, params).build
shout.save || flash.alert = "Could not shout."
redirect_to dashboard_url
end
end
Add the following routes:
%w(photo_shouts text_shouts).each do |type|
resources type, only: [:create],
controller: :shouts,
defaults: { shout_type: type.singularize }
end
Then delete unneeded routes and controllers.
I would love any feedback.
Thanks!