リファクタリングしてみた①

バージョン

・ruby 2.5.7
・Rails 5.2.4.3

ポートフォリオのコードをリファクタリング

まず、現状はというと下記の通り。ひどい、、。ほぼ同じ事を書いているアクション名が8つ存在している。まず、そのdate1からdate8のアクションをリファクタリングしてみる。

user_boke_tukkomi_controller.rb

class Users::UserBokeTukkomisController < ApplicationController
  before_action :authenticate_user!

  def index
    @genres = Genre.all
  end

  def show
    @genre = Genre.find(params[:id])
    @script = Script.new
    @script.name = @genre.name
    @script.user_id = current_user.id
    if @script.save
      redirect_to users_user_boke_tukkomis_date1_path(script_id: @script.id, genre_id: @genre.id )
    else
      redirect_to users_user_boke_tukkomis_path
    end
  end

  def date1
    @userboketukkomi = UserBokeTukkomi.new
    @boke_tukkomis = BokeTukkomi.where(page: 1, genre_id: params[:genre_id]).select(:furi,:boke,:tukkomi).distinct
    @script_id = params[:script_id]
    @genre_id = params[:genre_id]
  end

  def date2
    @userboketukkomi = UserBokeTukkomi.new
    @boke_tukkomis = BokeTukkomi.where(page: 2, genre_id: params[:genre_id]).select(:furi, :boke,:tukkomi).distinct
    @script_id = params[:user_boke_tukkomi][:script_id]
    @genre_id = params[:genre_id]

    parse_user_boke_tukkomi = params[:user_boke_tukkomi][:user_boke_tukkomi].split(",")
    user_boke_tukkomi = UserBokeTukkomi.new(boke: parse_user_boke_tukkomi[0],tukkomi: parse_user_boke_tukkomi[1], script_id: params[:user_boke_tukkomi][:script_id], furi: params[:user_boke_tukkomi][:furi])
      unless user_boke_tukkomi.save!
        redirect_to users_user_boke_tukkomis_date1_path
      end
  end

  def date3
    @userboketukkomi = UserBokeTukkomi.new
    @boke_tukkomis = BokeTukkomi.new
    @boke_tukkomis = BokeTukkomi.where(page: 3, genre_id: params[:genre_id]).select(:furi, :boke,:tukkomi).distinct
    @script_id = params[:user_boke_tukkomi][:script_id]
    @genre_id = params[:genre_id]

    parse_user_boke_tukkomi = params[:user_boke_tukkomi][:user_boke_tukkomi].split(",")
    user_boke_tukkomi = UserBokeTukkomi.new(boke: parse_user_boke_tukkomi[0],tukkomi: parse_user_boke_tukkomi[1], script_id: params[:user_boke_tukkomi][:script_id], furi: params[:user_boke_tukkomi][:furi])
      unless user_boke_tukkomi.save!
        redirect_to users_user_boke_tukkomis_date2_path
      end
  end

  def date4
    @userboketukkomi = UserBokeTukkomi.new
    @boke_tukkomis = BokeTukkomi.new
    @boke_tukkomis = BokeTukkomi.where(page: 4, genre_id: params[:genre_id]).select(:furi,:boke,:tukkomi).distinct
    @script_id = params[:user_boke_tukkomi][:script_id]
    @genre_id = params[:genre_id]

    parse_user_boke_tukkomi = params[:user_boke_tukkomi][:user_boke_tukkomi].split(",")
    user_boke_tukkomi = UserBokeTukkomi.new(boke: parse_user_boke_tukkomi[0],tukkomi: parse_user_boke_tukkomi[1], script_id: params[:user_boke_tukkomi][:script_id], furi: params[:user_boke_tukkomi][:furi])
      unless user_boke_tukkomi.save!
        redirect_to users_user_boke_tukkomis_date3_path
      end
  end

  def date5
    @userboketukkomi = UserBokeTukkomi.new
    @boke_tukkomis = BokeTukkomi.new
    @boke_tukkomis = BokeTukkomi.where(page: 5, genre_id: params[:genre_id]).select(:furi,:boke,:tukkomi).distinct
    @script_id = params[:user_boke_tukkomi][:script_id]
    @genre_id = params[:genre_id]

    parse_user_boke_tukkomi = params[:user_boke_tukkomi][:user_boke_tukkomi].split(",")
    user_boke_tukkomi = UserBokeTukkomi.new(boke: parse_user_boke_tukkomi[0],tukkomi: parse_user_boke_tukkomi[1], script_id: params[:user_boke_tukkomi][:script_id], furi: params[:user_boke_tukkomi][:furi])
      unless user_boke_tukkomi.save!
        redirect_to users_user_boke_tukkomis_date4_path
      end
  end

  def date6
    @userboketukkomi = UserBokeTukkomi.new
    @boke_tukkomis = BokeTukkomi. new
    @boke_tukkomis = BokeTukkomi.where(page: 6, genre_id: params[:genre_id]).select(:furi,:boke,:tukkomi).distinct
    @script_id = params[:user_boke_tukkomi][:script_id]
    @genre_id = params[:genre_id]

    parse_user_boke_tukkomi = params[:user_boke_tukkomi][:user_boke_tukkomi].split(",")
    user_boke_tukkomi = UserBokeTukkomi.new(boke: parse_user_boke_tukkomi[0],tukkomi: parse_user_boke_tukkomi[1], script_id: params[:user_boke_tukkomi][:script_id], furi: params[:user_boke_tukkomi][:furi])
      unless user_boke_tukkomi.save!
        redirect_to users_user_boke_tukkomis_date5_path
      end
  end

  def date7
    @userboketukkomi = UserBokeTukkomi.new
    @boke_tukkomis = BokeTukkomi.new
    @boke_tukkomis = BokeTukkomi.where(page: 7, genre_id: params[:genre_id]).select(:furi,:boke,:tukkomi).distinct
    @script_id = params[:user_boke_tukkomi][:script_id]
    @genre_id = params[:genre_id]

    parse_user_boke_tukkomi = params[:user_boke_tukkomi][:user_boke_tukkomi].split(",")
    user_boke_tukkomi = UserBokeTukkomi.new(boke: parse_user_boke_tukkomi[0],tukkomi: parse_user_boke_tukkomi[1], script_id: params[:user_boke_tukkomi][:script_id], furi: params[:user_boke_tukkomi][:furi])
      unless user_boke_tukkomi.save!
        redirect_to users_user_boke_tukkomis_date6_path
      end
  end

  def date8
    logger.debug 'zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz'
    logger.debug request.referer
    logger.debug 'zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz'
    @userboketukkomi = UserBokeTukkomi.new
    @boke_tukkomis = BokeTukkomi.new
    @boke_tukkomis = BokeTukkomi.where(page: 8, genre_id: params[:genre_id]).select(:furi,:boke,:tukkomi).distinct
    @script_id = params[:user_boke_tukkomi][:script_id]
    @genre_id = params[:genre_id]

    parse_user_boke_tukkomi = params[:user_boke_tukkomi][:user_boke_tukkomi].split(",")
    user_boke_tukkomi = UserBokeTukkomi.new(boke: parse_user_boke_tukkomi[0],tukkomi: parse_user_boke_tukkomi[1], script_id: params[:user_boke_tukkomi][:script_id], furi: params[:user_boke_tukkomi][:furi])
      unless user_boke_tukkomi.save!
        redirect_to users_user_boke_tukkomis_date7_path
      end

    if request.referer.include?('date8')
    #シナリオテーブルにフリボケツッコミを保存する処理
    userboketukkomi = UserBokeTukkomi.where(script_id: @script_id)
    contents = []
    userboketukkomi.each do |ubt|
      contents << ubt.furi
      contents << ubt.boke
      contents << ubt.tukkomi
    end
    contents_new=contents.join("<br>")
    script = Script.find @script_id
    script.update(furiboketukkomi: contents_new)
    redirect_to users_scripts_path
  end
  end

  private
  def user_boke_tukkomi_params
    params.permit(:script_id, :furi, :boke, :tukkomi)
  end

  def script_params
    params.require(:script).permit(:user_id, :name, :furiboketukkomi)
  end

  def genre_params
    params.require(:genre).permit(:genre ,:name)
  end
end

##それぞれの処理で分ける処理は何か？同じ処理は何か？の視点で考える
まずこのdate1からdate8のアクションの処理内容を大まかに分類すると、
date1は新しくレコードをnewする処理。
date2からdate7は新しくレコードをnewする処理＋前のページから渡ってきたカラムを受け取って、レコードを保存する処理。
date8は前のページから渡ってきたカラムを受け取って、レコードを保存する処理＋また別のレコードを保存する処理。
なので、date2〜date7はまとめれそうだと考えた。

##同じ処理をまとめるにはどうしたらいいか
今回の場合、リファクタリングしたくてもそれを阻んでいたのはこのpageカラムの存在。
date2~7はpageの値が変わっているだけで、後の処理はredirect先が違うくらいでコードは一緒だった。

user_boke_tukkomi_controller.rb

 @boke_tukkomis = BokeTukkomi.where(page: 2, genre_id: params[:genre_id])

##ページ遷移するごとに値を増やしていく
pageの値を自分で書くのではなく、ページが変わるごとに値が＋１ずつされるようにコードかけばいいのではと考えた。
その為にまずdate1で＠conto_page = 1とインスタンス変数を作る。

user_boke_tukkomi_controller.rb

  def date1
    @usersjoke = UsersJoke.new
    @adminsjoke = AdminsJoke.where(page: 1, genre_id: params[:genre_id])
    @joke_book_id = params[:joke_book_id]
    @genre_id = params[:genre_id]
    @conto_page = 1
  end

次に、ビュー側で値を受取り、form_withでconto_page: ＠conto_pageでdate2に＠conto_pageを渡す。

date1.html.erb

<%= form_with(model: @userboketukkomi ,url: users_user_boke_tukkomis_date2_path(script_id: @script_id, genre_id: @genre_id, conto_page: @conto_page),method: :post, local: true) do |f| %>

最後に、受け取ったparams[:conto_page].to_i == 8 ならdate8に行くようにredirectして、それ以外ならconto_pageが＋１ずつ増えて行く処理をして

user_boke_tukkomi_controller.rb

    if params[:conto_page].to_i == 8
      @script_id = params[:users_boke_tukkomi][:script_id]
      @genre_id = params[:genre_id]
      redirect_to users_user_boke_tukkomi_date8_path(script_id: @script_id, genre_id: @genre_id)
    else
      @conto_page = params[:conto_page].to_i+1
    end

##リファクタリング完成！
無事にdate3~date7のアクションは消す事が出来ました。
さっきよりコード量が４割ほど削減が出来ました。