Edited at

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


初めに

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文字少なくなりました。

おわり