画像販売サイトのひどすぎるコードのリファクタリングをする（少しマシにする）

初めに

Advent Calenderをすることにして使い方があっているかわからないけど自分のRUbyやRailsの技術を上げるために毎日何かしていこうと思う。

いま自分が作った画像販売サイトを就職の時に見せたりしているけどそれのリファクタリングする

とりあえず今のcontrollerのひどい部分をどうぞ

photos_controller.rb

class PhotosController < ApplicationController
  before_action :check_login,{only:[:new, :create, :pay, :purchase_form, :download, :my_index, :delete]}
  before_action :store_location,{only:[:index, :new, :create, :show]}
  protect_from_forgery except: :pay

  def index
    @photos = Photo.all.page(params[:page]).per(20)
  end

  def new
    @photo = Photo.new
  end

  def create 
    if params[:image]
      @photo = Photo.new(user_id: session[:user_id], category: params[:category], name: params[:name], title: params[:title], size: params[:size], color: params[:color], price: params[:price], tag: params[:tag])
      @photo.save
      if @photo.save
        @photo.name = "#{@photo.id}.jpg"
        @photo.save
        image = params[:image]
        File.binwrite("public/images/#{@photo.name}", image.read)
        redirect_to("/photos/index")
      else 
        render("photos/new")
      end 
    else 
      @photo = Photo.new(user_id: session[:user_id], category: params[:category], name: params[:name], title: params[:title], size: params[:size], color: params[:color], price: params[:price], tag: params[:tag])
      render("photos/new")
    end 
  end 

  def show
  end

  def pay
    @photo = Photo.find_by(id: params[:id])
    if Boughtcollection.find_by(user_id: session[:user_id], photo_id: @photo.id)
      flash[:warn] = "既に購入しています。"
      redirect_to("/photos/index")
    else 
      flash[:warn] = "画像を購入しました。"
      Payjp.api_key = 'sk_test_9436e64da148290153f01c01'
      charge = Payjp::Charge.create(
      :amount => @photo.price,
      :card => params['payjp-token'],
      :currency => 'jpy',
    )
      f = File.open("./public/images/#{@photo.name}", 'r+b')
      bought = Boughtcollection.new(user_id: session[:user_id], photo_id: @photo.id, subphoto: f, photo_user_id: @photo.user_id, name: @photo.name, title: @photo.title, size: @photo.size, color: @photo.color, price: @photo.price, tag: @photo.price)
      bought.save 
      sold = AllSold.new(user_id: @photo.user_id, money: @photo.price, complete: false)
      sold.save
      redirect_to("/photos/index")
    end 
  end

  def purchase_form
    @photo = Photo.find_by(id: params[:id])
  end 

  def search 
    @photos = Photo.where("color like ? or category like ? or title like ? or tag like ?",'%' + params[:keyword] + '%', '%' + params[:keyword] + '%', '%' + params[:keyword] + '%', '%' + params[:keyword] + '%').page(params[:page]).per(20)
  end 

  def search_color 
    @photos = Photo.where(color: params[:color]).page(params[:page]).per(20)
  end 

  def search_category 
    @photos = Photo.where(category: params[:category]).page(params[:page]).per(20)
  end 

  def download
    @photo = Boughtcollection.find_by(photo_id: params[:id])
    send_file "public/images/#{@photo.name}", :filename => "photoray-#{@photo.id}-18#{@photo.created_at}.jpg", :type => 'image/jpg'
  end

  def my_index
    @photos = Photo.where(user_id: session[:user_id]).page(params[:page]).per(10)
  end 

  def delete
    @photo = Photo.find_by(id: params[:id])
    @photo.destroy
    redirect_to('/photos/my_index')
  end 


end

Payjpというクレジットカード決済サービスのApikeyが普通に書いてある。

api

Payjp.api_key = 'sk_test_9436e64da148290153f01c01'

この部分です。

これはテスト用ですしばれても構わないんですがデプロイする際にはどうにかしないといけない...。

ですが今回はあくまでリファクタリングなのでとりあえず後回しで！

Photos#createのコード

画像作成のところを
user.photos.create(photo_params)にして

photos_controller.rb

private

def photo_params
  params.require(:photo).permit(***)
end

みたいにした

一つ問題になったことは画像ファイルをどのようにしてpublicに保存するかということ

とりあえずimageobjectというカラムをphotosに追加してストロングパラメータにもその値を追加、そして

imageobject.rb

def create 
      user = User.find session[:user_id]
      @photo = user.photos.create(photo_params)
      image = photo_params[:imageobject]
      if @photo.save
        @photo.name = "#{@photo.id}.jpg"
        @photo.save
        File.binwrite("public/images/#{@photo.name}", image.read)
        redirect_to("/photos/index")
      else 
        render("photos/new")
      end 
  end 

こんな感じに。

user = User.find session[:user_id]はcurrent_userをapplication_controllerで設定して@current_user.photos.createにする

次にPhotos#pay

とりあえずBoughtcollectionとphotoをアソシエーションでつなげることにしたらtitle: @photo.titleやらなんやら書かなくてすむ。

Payjp::Charge.create....のところはphotos_helperに

photos_helper.rb

def charge(photo)
        Payjp::Charge.create(
        :amount => photo.price,
        :card => params['payjp-token'],
        :currency => 'jpy')
end

と書く

photos_controllerにphotos_helperをインクルードして使います。

結果こんな感じ

photos_controller.rb

  def pay
    @photo = Photo.find_by(id: params[:id])
    if Boughtcollection.find_by(user_id: session[:user_id], photo_id: @photo.id)
      flash[:warn] = "既に購入しています。"
      redirect_to("/photos/index")
    else 
      flash[:warn] = "画像を購入しました。"
      Payjp.api_key = 'sk_test_9436e64da148290153f01c01'
      charge = charge(@photo)
      f = File.open("./public/images/#{@photo.name}", 'r+b')
      bought = Boughtcollection.new(user_id: session[:user_id], photo_id: @photo.id, subphoto: f, photo_user_id: @photo.user_id)
      bought.save 
      sold = AllSold.new(user_id: @photo.user_id, money: @photo.price, complete: false)
      sold.save
      redirect_to("/photos/index")
    end 
  end

まだ長いしapi_keyが気になるけどとりあえずは良しとする

Photo#search

whereが信じられないくらい長くなって画面からはみ出してる(-_-;)

'%' + params[:keyword] + '%'を何回も書くのはあほらしいので変える

それでこんな感じ

photos_controller.rb

  def search 
    f = '%' + params[:keyword] + '%'
    @photos = Photo.where("category like ? or title like ? or tag like ?",f,f,f).page(params[:page]).per(20)
  end

まあこれでもいいんだけどおしゃれにしたいからスコープを使う

photo.rb

scope :photo_search, ->(x) { where("category like ? or title like ? or tag like ?",f,f,f)}

として

photos_controller.rb

  def search 
    f = '%' + params[:keyword] + '%'
    @photos = Photo.photo_search(f).page(params[:page]).per(20)
  end 

短くなった

Photos_controller

リファクタリングしてこんな感じになりました。

photos_controller.rb

class PhotosController < ApplicationController
  before_action :check_login,{only:[:new, :create, :pay, :purchase_form, :download, :my_index, :delete]}
  before_action :store_location,{only:[:index, :new, :create, :show]}
  protect_from_forgery except: :pay
  attr_accessor :imageobject

  def index
    @photos = Photo.all.page(params[:page]).per(20)
  end

  def new
    @photo = Photo.new
  end

  def create 
      @photo = @current_user.photos.create(photo_params)
      image = photo_params[:imageobject]
      if @photo.save
        @photo.name = "#{@photo.id}.jpg"
        @photo.save
        File.binwrite("public/images/#{@photo.name}", image.read)
        redirect_to("/photos/index")
      else 
        render("photos/new")
      end 
  end 

  def show
  end

  def pay
    @photo = Photo.find_by(id: params[:id])
    if Boughtcollection.find_by(user_id: session[:user_id], photo_id: @photo.id)
      flash[:warn] = "既に購入しています。"
      redirect_to("/photos/index")
    else 
      flash[:warn] = "画像を購入しました。"
      Payjp.api_key = 'sk_test_9436e64da148290153f01c01'
      charge = charge(@photo)
      f = File.open("./public/images/#{@photo.name}", 'r+b')
      bought = Boughtcollection.new(user_id: session[:user_id], photo_id: @photo.id, subphoto: f, photo_user_id: @photo.user_id)
      bought.save 
      sold = AllSold.new(user_id: @photo.user_id, money: @photo.price, complete: false)
      sold.save
      redirect_to("/photos/index")
    end 
  end

  def purchase_form
    @photo = Photo.find_by(id: params[:id])
  end 

  def search 
    f = '%' + params[:keyword] + '%'
    @photos = Photo.photo_search(f).page(params[:page]).per(20)
  end 

  def search_color 
    @photos = Photo.where(color: params[:color]).page(params[:page]).per(20)
  end 

  def search_category 
    @photos = Photo.where(category: params[:category]).page(params[:page]).per(20)
  end 

  def download
    @photo = Boughtcollection.find_by(photo_id: params[:id])
    send_file "public/images/#{@photo.name}", :filename => "photoray-#{@photo.id}-18#{@photo.created_at}.jpg", :type => 'image/jpg'
  end

  def my_index
    @photos = Photo.where(user_id: session[:user_id]).page(params[:page]).per(10)
  end 

  def delete
    @photo = Photo.find_by(id: params[:id])
    @photo.destroy
    redirect_to('/photos/my_index')
  end 


  private 

  def photo_params 
    params.require(:photo).permit(:user_id, :category, :name, :title, :size, :color, :price, :tag, :imageobject)
  end 

end

まあ少しはましになりました。

つづいてusers_controller

全体像はこんな感じ

users_controller.rb

class UsersController < ApplicationController
  before_action :store_location,{only:[:index, :show]}
  before_action :check_login, {only: [:edit, :mypage, :myinfo, :all_sold_money, :needpay]}


  def index
    @users = User.all
  end

  def show
    @user = User.find_by(id: params[:id])
  end

  def add
    @user = User.new
  end

  def create 
    @user = User.create(user_params)
    if @user.save
      session[:user_id] = @user.id
      redirect_to(session[:return_to] || default)
      session[:return_to] = nil
    else 
      render("users/add")
    end
  end 

  def edit
    @user = User.find_by(id: session[:user_id])
  end

  def update 
    @user = User.find_by(id: session[:user_id])
    @user = User.update(user_params)
    redirect_to('/users/mypage')
  end 

  def login
    @user = User.find_by(mail: params[:mail], password: params[:password])
    if @user
      session[:user_id] = @user.id
      redirect_to(session[:return_to])
      session[:return_to] = nil
    else 
      render('login_form')
    end 
  end 

  def logout
    if session[:user_id]
      session[:user_id] = nil
    end 
    redirect_to('/home/top')
  end 

  def login_form
  end

  def mypage 
    @user = User.find_by(id: session[:user_id])
  end 

  def my_info
    @user = User.find_by(id: session[:user_id]) 
    money = AllSold.where user_id: session[:user_id], complete: false
    allmoney = 0
    money.each do |m|
      allmoney += m.money 
    end 
    @allm = allmoney

    sold = AllSold.where user_id: session[:user_id], complete: true
    allpaymoney = 0
    sold.each do |m|
      allpaymoney += m.money 
    end 
    @allp = allpaymoney

    @totalpay = @allp + @allm
  end 

  def all_sold_money
    money = AllSold.where user_id: session[:user_id], complete: false
    allmoney = 0
    money.each do |m|
      allmoney += m.money 
    end 
    @allm = allmoney

    sold = AllSold.where user_id: session[:user_id], complete: true
    allpaymoney = 0
    sold.each do |m|
      allpaymoney += m.money 
    end 
    @allp = allpaymoney

    @totalpay = @allp + @allm

  end 

  def needpay 
    @user = User.find_by(id: session[:user_id])
    money = AllSold.where user_id: session[:user_id], complete: false 
    allmoney = 0
    money.each do |m|
      allmoney += m.money 
    end 
    if allmoney > 0
      @user.pay = allmoney
      @user.save
      money.each do |m|
        m.complete = true 
        m.save
      end 
      InquiryMailer.send_mail(@user).deliver_now
      InquiryMailer.send_ray(@user).deliver_now
      money = AllSold.where user_id: session[:user_id], complete: false
      redirect_to('/users/all_sold_money')
    else 


    flash[:warn] = "請求できるお金はありません。"
    redirect_to("/photos/index")
    end 
  end 

  def bank 
    @user = User.find_by(id: session[:user_id])
  end 

  def bankedit
    @user = User.find_by(id: session[:user_id])
  end 

  def bankupdate 
    @user = User.find_by(id: session[:user_id])
    @user = User.update(user_params)
    redirect_to('/users/bank')
  end 

  def kiyaku 
  end 

  private 
  def user_params
    params.require(:user).permit(:name, :mail, :password, :createrName, :phone, :address, :bank, :bornyear, :bornmonth, :bornday, :bankname, :bankkind, :bankcode, :banknumber, :meigitop, :meigisecond)
  end 

end

お金の処理系をHelperで行う

ここで過去に請求したお金や請求できる金額を計算したりしています。

users_controller.rb

  def my_info
    @user = User.find_by(id: session[:user_id]) 
    money = AllSold.where user_id: session[:user_id], complete: false
    allmoney = 0
    money.each do |m|
      allmoney += m.money 
    end 
    @allm = allmoney

    sold = AllSold.where user_id: session[:user_id], complete: true
    allpaymoney = 0
    sold.each do |m|
      allpaymoney += m.money 
    end 
    @allp = allpaymoney

    @totalpay = @allp + @allm
  end 

  def all_sold_money
    money = AllSold.where user_id: session[:user_id], complete: false
    allmoney = 0
    money.each do |m|
      allmoney += m.money 
    end 
    @allm = allmoney

    sold = AllSold.where user_id: session[:user_id], complete: true
    allpaymoney = 0
    sold.each do |m|
      allpaymoney += m.money 
    end 
    @allp = allpaymoney

    @totalpay = @allp + @allm

  end 

  def needpay 
    @user = User.find_by(id: session[:user_id])
    money = AllSold.where user_id: session[:user_id], complete: false 
    allmoney = 0
    money.each do |m|
      allmoney += m.money 
    end 
    if allmoney > 0
      @user.pay = allmoney
      @user.save
      money.each do |m|
        m.complete = true 
        m.save
      end 
      InquiryMailer.send_mail(@user).deliver_now
      InquiryMailer.send_ray(@user).deliver_now
      money = AllSold.where user_id: session[:user_id], complete: false
      redirect_to('/users/all_sold_money')
    else 


    flash[:warn] = "請求できるお金はありません。"
    redirect_to("/photos/index")
    end 
  end 

UsersHelperに

users_helper.rb

module UsersHelper

    def all_money
        money = AllSold.where user_id: session[:user_id], complete: false
        allmoney = 0
        money.each do |m|
            allmoney += m.money 
        end
        return allmoney 
    end 

    def all_pay_money
        sold = AllSold.where user_id: session[:user_id], complete: true
        allpaymoney = 0
        sold.each do |m|
            allpaymoney += m.money 
        end
        return allpaymoney
    end

end

こんな感じにしてこれをUsersControllerでインクルードして使う

あとはUser.find_by session[:user_id]は@current_userにしました

それで結果としてusers_controllerはこんな感じになりました。

users_controller.rb

class UsersController < ApplicationController
  include UsersHelper

  before_action :store_location,{only:[:index, :show]}
  before_action :check_login, {only: [:edit, :mypage, :myinfo, :all_sold_money, :needpay]}


  def index
    @users = User.all
  end

  def show
    @user = User.find_by(id: params[:id])
  end

  def add
    @user = User.new
  end

  def create 
    @user = User.create(user_params)
    if @user.save
      session[:user_id] = @user.id
      redirect_to(session[:return_to] || default)
      session[:return_to] = nil
    else 
      render("users/add")
    end
  end 

  def edit
    @user = @current_user
  end

  def update 
    @user = @current_user
    @user = User.update(user_params)
    redirect_to('/users/mypage')
  end 

  def login
    @user = User.find_by(mail: params[:mail], password: params[:password])
    if @user
      session[:user_id] = @user.id
      redirect_to(session[:return_to])
      session[:return_to] = nil
    else 
      render('login_form')
    end 
  end 

  def logout
    if session[:user_id]
      session[:user_id] = nil
    end 
    redirect_to('/home/top')
  end 

  def login_form
  end

  def mypage 
    @user = @current_user
  end 

  def my_info
    @user = @current_user 
    @allm = all_money 
    @allp = all_pay_money
    @totalpay = @allp + @allm
  end 

  def all_sold_money 
    @allm = all_money 
    @allp = all_pay_money

    @totalpay = @allp + @allm

  end 

  def needpay 
    @user = @current_user 
    money = AllSold.where user_id: session[:user_id], complete: false
    allmoney = all_money
    if allmoney > 0
      @user.pay = allmoney
      @user.save
      money.each do |m|
        m.complete = true 
        m.save
      end 
      InquiryMailer.send_mail(@user).deliver_now
      InquiryMailer.send_ray(@user).deliver_now
      redirect_to('/users/all_sold_money')
    else 

    flash[:warn] = "請求できるお金はありません。"
    redirect_to("/photos/index")
    end 
  end 

  def bank 
    @user = @current_user
  end 

  def bankedit
    @user = @current_user
  end 

  def bankupdate 
    @user = @current_user
    @user = User.update(user_params)
    redirect_to('/users/bank')
  end 

  def kiyaku 
  end 

  private 
  def user_params
    params.require(:user).permit(:name, :mail, :password, :createrName, :phone, :address, :bank, :bornyear, :bornmonth, :bornday, :bankname, :bankkind, :bankcode, :banknumber, :meigitop, :meigisecond)
  end 

end

いまいち少なくなった気がしないので…

あんま少なくなった気がしないので文字数を計算して比較してみる

使うコードはこんな感じ

count.rb

Encoding.default_external = 'UTF-8'

data = ""

while line = ARGF.gets
    data.concat(line.gsub(/[\s　]/, ''))
end

puts data.length

これを使ったところ
変更前のphotos_controller.rbの文字数は2510文字、users_controller.rbの文字数は2447文字
合計4957文字でした

変更後はphotos_controller.rbの文字数は1991文字、users_controller.rbの文字数は1795文字
合計3786文字

ということで1171文字少なくなりました。

おわり