こんにちは、エンジニアの麦倉です。

このたび、株式会社ギフティは Kaigi on Rails 2023 に Ruby Sponsors として参加させていただいております。イベントの2日間両日のスポンサーブースにて、"Code Review Challenge" というちょっとした Rails クイズのような企画を出展しています。この記事では、2日目に出題した3問の正解発表を行っていきます。ブースにお越しいただいた方、本当にありがとうございます！

なお、1日目の正解発表はこちらのページをご覧ください。

また、別途 Kaigi on Rails 2023 の参加レポートを後日アップする予定なので、そちらもお楽しみに！

第 1 問 : 利口な UI (難易度: ★☆☆)

設問はこちら: https://esa-pages.io/p/sharing/2867/posts/8746/13d1a04a07f29599f6fa.html

正解は (3) です！

この設問は、MVCモデルのそれぞれの責務を考え、Viewにどのようなロジックは置くべきかを問うものでした。今回のコードでは、「有効期限内かつ未利用であること」というビジネスロジックが置かれています。このようなロジックはビジネスに密接に紐づいたロジックであり、ViewではなくModelに置くべきです。なぜならViewにおいてしまうと、下記のような問題が発生しうるためです。

Viewに対してテストを書く際に、全てのViewに対してビジネスロジックに対応するテストを書く必要がでてくる
様々なViewに同じようなビジネスロジックが重複してしまい、そのロジックに修正が入った際に全てのロジックを修正する必要がでてきて変更コストが高くなる

ということで、正解は (3) の有効期限内かつ未利用であることを調べるロジックがビューに実装されているでした。

第 2 問 : ステージング環境の構築 (難易度: ★★☆)

設問はこちら: https://esa-pages.io/p/sharing/2867/posts/8747/748511a906ce39ef1c4f.html

正解は (1) です！

この設問は、本番環境となるべく似せた設定のステージング環境をつくるためにはどのような点を気をつけるべきか、について問うものでした。仮に、stgでのアプリケーション設定がconfig/environments/staging.rb、config/environments/production.rbのように別々になってしまうと、本番環境とステージング環境のアプリケーション設定が片方だけパラメータが違う、等の可能性も考えられてしまうため、stagingとproductionとの設定が同一であることが保証されなくなってしまいます。「本番環境と似たもの」と考えると、アプリケーション設定は同一にした上で、環境変数を分けることで内部処理の制御をすべきだろうということから、正解は (1) でした。

第 3 問 : パフォーマンス改善 (難易度: ★★★)

設問はこちら: https://esa-pages.io/p/sharing/2867/posts/8748/45f50396b5ffe3cf72b0.html

こちらは記述式の回答でしたが、想定解は特に以下のような点でした。

(1) モデルのコードにおいて、Gift.urls は pluck で済む
(2) ビューのコードにおいて、brand_name で N+1 が起きている
(3) メンテ手順のコードにおいて、find_each を使っていない

以下、各想定解についての簡単な解説です。

(1) について

Gift.urlsにおいて、all.map { |gift| gift.url }のように記述されています。しかしこれでは、SELECT * FROM giftsでGiftのすべてのカラム情報を取得してきた後、ActiveRecord オブジェクトを生成してurlを取得するような構成となっています。つまりurlしか必要ないのに、Rails側のメモリ使用量も、SQLサーバと Railsサーバとの通信帯域使用量も不必要に消費するような構成になっています。ここでall.pluck(:url)にすることによって、SELECT url FROM giftsのような情報を取得することになるため、不必要な情報のやり取りやメモリ確保を防ぐことが出来ます。

(2) について

app/views/gifts/index.html.erbにおいて、@gifts.each do |gift|の中でgift.brand_nameを参照しています。つまりSELECT * FROM gifts WHERE user_id = user_idのようなクエリを発行した後に、ここで取得された各giftに対して更にSELECT name FROM brands WHERE id = giftに入っているbrand_idのようなクエリを発行する、つまりN+1問題が発生してしまっています。これを防ぐためにincludes(:brand)をつけることによって、SELECT * FROM gifts FROM gifts LEFT JOIN brands ON gifts.brand_id = brands.id WHERE user_id = user_idのようなクエリでgiftとbrand情報をまとめて取ってきてあげるべきです。

(3) について

メンテ手順のコードにおいてGift.all.each do |gift|のようなコードが書かれています。Giftのレコード数が少数であれば良いものの、多い場合にこのようにしてしまうとgifts情報をまとめて取ってきてしまうため、メモリ不足等のエラーが発生する可能性があります。そこでGift.find_each do |gift|とすることによって、ユーザが決めた件数(デフォルト値では1000件)ごとに取得するようにクエリを発行してくれるため、メモリの消費量が減らせるため、find_eachを使うべきです。