← Back to Upcase

Week 3: PhotoShoutsController and TextShoutsController refactor


(Jason Decker) #1

Good morning,

Here is my take on the PhotoShoutsController and TextShoutsController refactor:

https://gist.github.com/curiositypaths/4d65972950c07f677a0d

Jason


(Justin Leveck) #2

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.


(Jon Seidel) #3

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…


(Aaron Renner) #4

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.


(Dwayne R. Crooks) #5

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!


(Shawndromat) #6

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?


(turino) #7

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!