【第1回】いにしえの個人開発アプリをリファクタする
今回のお題
技術ブログのネタをどうしようかと迷った挙句、1年前にRailsで開発したアプリに手をつけることにした。
改良ポイントは果てしなくあるので、シリーズものにできる。我ながらナイスアイデア。
制作期間は2021年11月~2022年2月で、未経験からWebエンジニアに転職するためのポートフォリオにしていた。
1年弱の実務経験を経た今の状態でコードを眺めてみると、色々な気づきがあって面白い。
長くなりそうなので前置きはこの辺にして、早速コードについて書いていきたい。
本題
▼今回作成したプルリク
対象はアニメの最新話が追加されたらLINE通知を送るバッチ処理。
before
手をつけたいのは、深すぎるネストの最深部にある
if LineNotification.can_notify?の箇所だ。
masters.each do |master|
@contents = Content.where(master_id: master.id, line_flag: true)
if @contents.present?
@contents.each do |content|
if content.episode < master.episode
line_users = content.users.where.not(uid: nil)
line_users.each do |user|
message = {
type: 'text',
text: "#{master.title}の#{master.episode}話が公開されました！\n#{master.url}"
}
client.push_message(user.uid, message) if LineNotification.can_notify?
LineNotification.create_record(master, month)
p "LINE通知:#{content.title}をuser_id:#{user.id}さんに送信しました"
end
end
end
end
end
このクラスメソッドはModel内で以下のように定義されている。
class LineNotification < ApplicationRecord
belongs_to :master
counter_culture :master
scope :monthly_total, -> { where(month: Date.today.month) }
def self.can_notify?
monthly_total.size < 1000
end
def self.create_record(master, month)
create(master_id: master.id, month: month)
end
end
monthly_totalというscopeで、今月の通知数を取得し、1000件を超えていないかを判定している。
課金の発生を避けるためだ。（タダ乗りしてごめんなさい）
クエリの発行数を減らしたい
ここで気づくべきは、each処理の中で同じクエリを何度も発行してしまっていることだ。
line_users.each do |user|
message = {
type: 'text',
text: "#{master.title}の#{master.episode}話が公開されました！\n#{master.url}"
}
client.push_message(user.uid, message) if LineNotification.can_notify?
LineNotification.create_record(master, month)
p "LINE通知:#{content.title}をuser_id:#{user.id}さんに送信しました"
end
通知対象のユーザー数が増えれば何十回、何百回と同じクエリが発行されてしまう。
そこで、monthly_totalの実行は1回にして、通知処理を実行するごとに、カウントを1ずつ増やすようにしてみることにした。
（他にもベターなやり方はおそらくあって、そもそもこの深すぎるネストをなんとかするべきでは？という大勢の声が聞こえる）
after
結果として出来上がったのが以下のコードだ。
class LineNotification < ApplicationRecord
attribute :total_count, type: Integer, default: 1000
belongs_to :master
counter_culture :master
scope :monthly_total, -> { where(month: Time.zone.today.month) }
def self.create_record(master, month)
create(master_id: master.id, month: month)
end
def can_notify?
total_count < 1000
end
end
#月に1000件以上プッシュ通知を送ると課金されるため通知数を管理
line_notification = LineNotification.new
line_notification.total_count = LineNotification.monthly_total.size
catch(:exit) do
masters.each do |master|
@contents = Content.where(master_id: master.id, line_flag: true)
if @contents.present?
@contents.each do |content|
if content.episode < master.episode
line_users = content.users.where.not(uid: nil)
line_users.each do |user|
if line_notification.can_notify?
message = {
type: 'text',
text: "#{master.title}の#{master.episode}話が公開されました！\n#{master.url}"
}
client.push_message(user.uid, message)
LineNotification.create_record(master, month)
p "LINE通知:#{content.title}をuser_id:#{user.id}さんに送信しました"
line_notification.total_count += 1
else
throw :exit
end
end
end
end
end
end
end
差分：https://github.com/inoway46/subani/pull/64/files
さらにネストが深くなってしまった。多重ループを抜ける方法は、throw→catchが良いと聞きまして・・・。
本題に戻ると、LineNotificationモデルにtotal_countという属性値を追加したことに注目いただきたい。
（デフォルト値を1000にしたのは何が何でもお金を払いたくないからです）
class LineNotification < ApplicationRecord attribute :total_count, type: Integer, default: 1000
バッチ処理内では、ネストの森の入り口で以下のように通知数を取得するようにした。
line_notification = LineNotification.new line_notification.total_count = LineNotification.monthly_total.size
ギリギリ1000件までは通知するようにしたいので、多少の無駄クエリは発生するが、
最深部のループでif line_notification.can_notify?を判定するようにしている。
line_users.each do |user|
if line_notification.can_notify?
message = {
type: 'text',
text: "#{master.title}の#{master.episode}話が公開されました！\n#{master.url}"
}
client.push_message(user.uid, message)
LineNotification.create_record(master, month)
p "LINE通知:#{content.title}をuser_id:#{user.id}さんに送信しました"
line_notification.total_count += 1
else
throw :exit
end
end
通知処理に入ったら、送信後にline_notification.total_count += 1でカウントを追加する。
1000件を超えている場合は、throw :exitでループを一気に抜ける。
まとめ
以上が今回の変更内容である。
コード量は増えてしまったが、問題のクエリの発行数は1回に済ませることができた。
ちなみに、単にtotal_count = LineNotification.monthly_total.sizeとせずにtotal_countをモデルの属性値として追加した理由は、
コードの可読性を上げるためだ。
if total_count < 1000よりif line_notification.can_notify?の方が処理の意図伝わりやすくない？
そんなことないっすかね・・・。（自信なくなってきた）
ともあれ、少しでもパフォーマンスを上げられたので、今回はこれで良しとする。
PRをマージする前にテストコードを追加しようと思っているので、次回はその件について記事にしようと思う。
（テストを書くのが面倒になったらテーマを変更する）
最後までお付き合いいただきありがとうございました。
〜第1回おわり〜
