UserCoursePractice.unstarted_practicesメソッドのリファクタリング#9000
Conversation
Walkthrough
Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as UserCoursePractice
participant QueryObj as UserUnstartedPracticesQuery
participant DB as Practice DB
Caller->>QueryObj: .new(user: @user).call
QueryObj->>DB: JOIN categories_practices/courses_categories\nWHERE course_id = user.course_id\nAND id NOT IN (learned_practice_ids)\nORDER BY positions
DB-->>QueryObj: 未着手プラクティス一覧
QueryObj-->>Caller: 結果を返却
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 RuboCop (1.76.1)app/models/user_course_practice.rbrubocop-minitest extension supports plugin, specify app/queries/user_unstarted_practices_query.rbrubocop-minitest extension supports plugin, specify Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
@tyrrell-IH |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/queries/user_unstarted_practices_query_test.rb (1)
21-46: 個別ステータステストの統合を検討してください既に包括的なテストが lines 6-19 で実装されているため、個別のステータステスト（lines 21-28, 30-37, 39-46）は重複している可能性があります。包括的なテストで十分なカバレッジが得られている場合は、これらの個別テストを削除してテストファイルを簡潔にすることを検討してください。
以下のdiffで個別テストを削除できます：
- test 'should exclude practices with started status' do - user = users(:komagata) - started_practice = practices(:practice1) - - result = UserUnstartedPracticesQuery.new(user:).call - - assert_not_includes result, started_practice - end - - test 'should exclude practices with submitted status' do - user = users(:komagata) - submitted_practice = practices(:practice4) - - result = UserUnstartedPracticesQuery.new(user:).call - - assert_not_includes result, submitted_practice - end - - test 'should exclude practices with complete status' do - user = users(:komagata) - completed_practice = practices(:practice2) - - result = UserUnstartedPracticesQuery.new(user:).call - - assert_not_includes result, completed_practice - end
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/models/user_course_practice.rb(1 hunks)app/queries/user_unstarted_practices_query.rb(1 hunks)test/queries/user_unstarted_practices_query_test.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rb
⚙️ CodeRabbit Configuration File
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/models/user_course_practice.rbtest/queries/user_unstarted_practices_query_test.rbapp/queries/user_unstarted_practices_query.rb
test/**/*
⚙️ CodeRabbit Configuration File
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/queries/user_unstarted_practices_query_test.rb
🧠 Learnings (2)
📓 Common learnings
Learnt from: ryufuta
PR: fjordllc/bootcamp#8992
File: app/models/correct_answer_notifier.rb:23-23
Timestamp: 2025-07-30T07:26:36.599Z
Learning: PRのスコープを維持することが重要であり、バグ修正PRにおいて、修正対象以外の機能（例：ルーティングテスト）の追加提案は適切ではない。そのような改善が必要な場合は別PRで対応すべきである。
Learnt from: kitarou888
PR: fjordllc/bootcamp#8277
File: test/models/searcher_test.rb:326-331
Timestamp: 2025-07-10T12:59:27.807Z
Learning: モデルテストにおいて、1つのクラスメソッドの複数の挙動を検証する場合、機能の異なる側面を同じテストメソッドでテストすることは、包括的なテストとして適切である。特に`only_me`のような機能では、異なる検索条件（空文字と具体的な検索語）を使い分けることで、より広範囲な動作保証が可能となる。
Learnt from: reckyy
PR: fjordllc/bootcamp#8356
File: db/fixtures/users.yml:823-826
Timestamp: 2025-07-15T12:33:04.885Z
Learning: fjordllc/bootcampプロジェクトでは、`trainee: true`は「研修に所属していたユーザー」全体を示すフラグであり、研修中のユーザーと研修を終了したユーザーの両方を含む。そのため、`trainee: true`と`training_completed_at`の併存は意図された仕様であり、排他的な関係ではない。
test/queries/user_unstarted_practices_query_test.rb (2)
Learnt from: kitarou888
PR: #8277
File: test/models/searcher_test.rb:326-331
Timestamp: 2025-07-10T12:59:27.807Z
Learning: モデルテストにおいて、1つのクラスメソッドの複数の挙動を検証する場合、機能の異なる側面を同じテストメソッドでテストすることは、包括的なテストとして適切である。特にonly_meのような機能では、異なる検索条件（空文字と具体的な検索語）を使い分けることで、より広範囲な動作保証が可能となる。
Learnt from: reckyy
PR: #8356
File: db/fixtures/users.yml:823-826
Timestamp: 2025-07-15T12:33:04.885Z
Learning: fjordllc/bootcampプロジェクトでは、trainee: trueは「研修に所属していたユーザー」全体を示すフラグであり、研修中のユーザーと研修を終了したユーザーの両方を含む。そのため、trainee: trueとtraining_completed_atの併存は意図された仕様であり、排他的な関係ではない。
🔇 Additional comments (7)
app/queries/user_unstarted_practices_query.rb (4)
3-4: Query オブジェクトパターンが適切に適用されています
Patterns::Queryを継承し、queries Practiceでターゲットモデルを明示的に指定している実装は rails-patterns gem の規約に従っており、良い設計です。
8-12: 初期化処理が適切に実装されていますユーザーとコースの設定が正しく行われており、親クラスの
super(relation)呼び出しも適切です。
14-21: クエリロジックが大幅に改善されています以下の改善点が確認できます：
- 複数のORクエリから単一のNOT INクエリへの変更で可読性が向上
- 明示的なソート順の指定によりデフォルトスコープへの依存を解消
- JOINの最適化により重複が削減
ソート順の
practice_position ASC, category_position ASCにより、プラクティスの位置とカテゴリの位置で適切に並び順が制御されています。
23-26: 学習ステータスの管理が一元化されています学習済みプラクティスIDの取得ロジックが1箇所に集約され、保守性が向上しています。
Learning.statuses.values_atを使用することで、ステータス値のハードコーディングを避けている点も良い実装です。test/queries/user_unstarted_practices_query_test.rb (2)
6-19: 包括的なテストケースが適切に実装されています1つのテストメソッドで複数の学習ステータス（started、submitted、complete、unstarted）の挙動を検証しており、retrieved learnings に基づくと、このような包括的なテストアプローチは適切です。未着手プラクティスの取得という1つの機能の異なる側面を効率的にテストできています。
48-56: ユーザー固有の動作テストが適切に実装されています異なるユーザー間で結果が異なることを確認するテストにより、クエリがユーザー固有の学習状況を正しく反映していることが検証されています。
app/models/user_course_practice.rb (1)
96-98: クエリオブジェクトパターンへのリファクタリングが適切に実装されています複雑なActiveRecordクエリを
UserUnstartedPracticesQueryに抽出し、コーディングガイドラインに従ってrails-patterns gemのQuery機能を活用しています。メモ化も維持されており、パフォーマンスへの配慮も適切です。このリファクタリングにより以下が達成されています：
- Fat Model の回避
- 複雑なクエリロジックの再利用可能性向上
- 可読性と保守性の向上
|
@jun-kondo |
|
@tyrrell-IH |
jun-kondo
force-pushed
the
refactor-unstarted_practices-method
branch
from
32cbe98 to
e1fd2d5
Compare
|
@tyrrell-IH |
|
@jun-kondo
現在の状態でレビューしますので大丈夫です。何かあったら連絡します〜 |
|
@jun-kondo もし難しければ、何とか時間を見て8/8(金)中にレビューします |
|
@tyrrell-IH |
tyrrell-IH
left a comment
tyrrell-IH
left a comment
There was a problem hiding this comment.
@jun-kondo
コメントしました！
間違ってる点、何を言ってるかよくわからない点があったら気兼ねなくおっしゃってください！
以下余談です。
今回の実装上は問題なく、あくまで仕様についての私見ですが、そもそもcategories_practices.positionの昇順が第一優先なのは少し変だなと思いました。
Mac OS Xカテゴリーの最初のプラクティスOS X Mountain Lionをクリーンインストールするを修了にしても、同カテゴリーにはまだ未着手のプラクティスが残っているので、アンカーの場所はMac OS Xのままで良いのでは？とちょっと思いました。
| .joins(categories: :courses_categories) | ||
| .where(courses_categories: { course_id: @course.id }) | ||
| .where.not(id: learned_practice_ids) | ||
| .select('practices.*') | ||
| .order('categories_practices.position ASC, courses_categories.position ASC') |
There was a problem hiding this comment.
categories_practices.positionを使うならcategories_practicesテーブルもJOINしていいような気がしました。
There was a problem hiding this comment.
こちら修正しました。
アソシエーション経由でSQLではJOINされているのですが、明示的に書いたほうがわかりやすいですね👀
ac987ca
| def learned_practice_ids | ||
| statuses = Learning.statuses.values_at(:started, :submitted, :complete) | ||
| Learning.where(user_id: @user.id, status: statuses).select(:practice_id) | ||
| end |
There was a problem hiding this comment.
サブクエリにするととても読みやすいと思いました。勉強になりました🙏
|
|
||
| require 'test_helper' | ||
|
|
||
| class UserUnstartedPracticesQueryTest < ActiveSupport::TestCase |
There was a problem hiding this comment.
未着手のプラクティスの並び（最初の要素が意図したものになっているか）を保証するようなテストも必要かなと思いました。
UserCoursePractice#unstarted_practicesが使われる箇所は
# UserCoursePractice#category_active_or_unstarted_practic
def category_active_or_unstarted_practice
if @user.active_practices.present?
category_having_active_practice
elsif unstarted_practices.present?
category_having_unstarted_practice
end
end
# UserCoursePractice#category_having_unstarted_practice
def category_having_unstarted_practice
unstarted_practices&.first&.categories&.first
endとなっており、
unstarted_practices&.first&.categories&.firstは
unstarted_practices内の最初のプラクティスの属するカテゴリーの位置をアンカーにしているので、意図した並びになっていないとアンカーになるカテゴリーの位置も変わってくるのかなと思いました🤔
There was a problem hiding this comment.
ご指摘ありがとうございます🙇♂️
UserCoursePractice#category_active_or_unstarted_practicには既存のモデルテストがありまして、こちらで目的を満たせていると考えたので今回のQueryObjectクラスのテストには含みませんでしたが如何でしょうか?👀
(以下補足)
テストは下記のようになっており、
1つめのアサーションではuser(:komagata)は着手中のプラクティスを持っているので、category_having_active_practiceを検証しています。
2つめのアサーションではusers(:machida)は着手中ステータスのプラクティスを持っていないので、category_having_unstarted_practiceの値、つまり先頭のプラクティスが所属するcategoryのidを検証しています。
test 'get category active or unstarted practice' do
komagata = @user_course_practice_komagata
assert_equal 917_504_053, komagata.category_active_or_unstarted_practice.id
machida = @user_course_practice_machida
practice1 = practices(:practice1)
user = users(:machida)
create_checked_product(user, practices(:practice1))
Learning.create!(
user: users(:machida),
practice: practice1,
status: :complete
)
assert_equal 685_020_562, machida.category_active_or_unstarted_practice.id
endThere was a problem hiding this comment.
すいません、そちらのテストは確認していませんでした🙏
おっしゃる通り、test 'get category active or unstarted practice' のuser: machidaのテストにおいて
test/fixtures/practices.ymlに登録されているデータ + テスト中に追加したpractice1(status: :complete)を合わせた上で、
どの未着手プラクティスが先頭→それがどのカテゴリーに属するかを検証してますね。
OKです🙆
| @user = user | ||
| @course = user.course | ||
| end | ||
|
|
||
| def query | ||
| relation | ||
| .joins(categories: :courses_categories) | ||
| .where(courses_categories: { course_id: @course.id }) |
There was a problem hiding this comment.
user.courseのように関連で引くと
SELECT "courses".* FROM "courses" WHERE "courses"."id" = $1 LIMIT $2 [["id", 829913840], ["LIMIT", 1]]
のように（初回だけ）クエリが発生するようです。
@course_id = user.course_idにして
.where(courses_categories: { course_id: @course_id })
でもいいのかなと思いました。
There was a problem hiding this comment.
こちら修正しました。
関連を使わずUserレコードのカラムの値を参照すれば余計なクエリ発行を減らせますね👀
25de41a
jun-kondo
force-pushed
the
refactor-unstarted_practices-method
branch
2 times, most recently
from
39a2c4c to
ac987ca
Compare
|
@tyrrell-IH 余談についてこちらもコメント頂きありがとうございます。この仕様に対するご指摘は正しいです🙆♂️ そのため別途issue#8998を作成し、私の方で修正対応します。 |
tyrrell-IH
left a comment
tyrrell-IH
left a comment
There was a problem hiding this comment.
確認しました、OKです！
1件コメント残しました。#9000 (comment)
test 'get category active or unstarted practice'については見ていなかったです。教えていただきありがとうございました🙏
jun-kondo
force-pushed
the
refactor-unstarted_practices-method
branch
from
ac987ca to
541f458
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/models/user_course_practice.rb (1)
96-98: 引数は@userではなくuserを使うとクラス内の表記が一貫します（軽微なnit）このファイル内では
attr_reader :userを多用しているので、ここも合わせると読みやすくなります。- @unstarted_practices ||= UserUnstartedPracticesQuery.new(user: @user).call + @unstarted_practices ||= UserUnstartedPracticesQuery.new(user: user).call
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
app/models/user_course_practice.rb(1 hunks)app/queries/user_unstarted_practices_query.rb(1 hunks)test/queries/user_unstarted_practices_query_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/queries/user_unstarted_practices_query.rb
- test/queries/user_unstarted_practices_query_test.rb
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
⚙️ CodeRabbit Configuration File
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/models/user_course_practice.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (2)
app/models/user_course_practice.rb (2)
96-98: Queryオブジェクト化とメモ化の導入は適切です/LGTM複雑なクエリをUserUnstartedPracticesQueryへ委譲し、Relationを返す形にしたのは、可読性・保守性・パフォーマンスの観点で良い判断です。単一クエリ化＋明示的なORDERも◎。
96-98: 確認結果 — 問題なし（Array専用の差集合演算は未検出）軽くリポジトリ内を検索しました。unstarted_practices の利用箇所は下記のみで、Array 固有の差集合演算（
-）は検出されませんでした。現状のままで影響はありません。
- app/models/user_course_practice.rb:38 —
elsif unstarted_practices.present?- app/models/user_course_practice.rb:97 —
def unstarted_practices(実装:UserUnstartedPracticesQuery.new(user: @user).call)- app/models/user_course_practice.rb:105 —
unstarted_practices&.first&.categories&.first
|
@tyrrell-IH |
|
@komagata |
- 変更前: user.courseの関連で参照していた - 変更後: user.course_idでuserレコードの外部キーを参照する - これにより関連をロードするためのクエリ発行を省略する
- CategoriesPracticeテーブルをjoinsメソッドの引数に追加 - 中間テーブルの関連付けをアソシエーションによる暗黙的なものから明示的に記載する
jun-kondo
force-pushed
the
refactor-unstarted_practices-method
branch
from
541f458 to
ed289be
Compare
3 participants
Issue
概要
UserCoursePractice.unstarted_practicesメソッドのrails-patterns gemのQueryオブジェクトのUserUnstartedPracticesQueryクラスにリファクタリングしました。戻り値としてプラクティスの配列を返していましたが、
ActiveRecord::Relationを返すようにしています。また変更前は
practicesを下記で取得していましたが、practicesのソートをCourseとPracticeの中間テーブルのデフォルトスコープに依存していたので、UserUnstartedPracticesQueryクラス内で明示的に並び順を指定するようにしました。issueで挙げられていた問題点の改善まとめ
可読性の低さ: 3つのORクエリーがチェーンされて非常に読みにくい
変更後: “OR”でつないでいた複数の条件を、配列を使った“IN”句へと書き換えてシンプルにした
重複したJOIN: 同じ
practices.joins(:learnings)が3回繰り返されている変更後:
learningテーブルへのJOINを行ず、条件を満たすpractice_idをlearningテーブルから直接取り出すサブクエリにすることでJOINを一箇所にまとめた保守性: 学習ステータスの追加時に3箇所を修正する必要がある
変更後: ステータスの管理を
learned_practice_idsメソッド1箇所に切り出しパフォーマンス: 効率的な1つのクエリーにまとめられる可能性
変更後
.where.not(id: learned_practice_ids)の利用で「未着手」のみを直接抽出unstarted_practicesを計算していたため、クエリが二回発生していた。変更後は一回のクエリで済むよう改善発行されるSQL
変更前
変更後
変更確認方法
ブランチをローカルに取り込む
ローカルサーバーの立ち上げ
ユーザーhatsuno`でログインhatsunotesttest※ ステータス変更時はナビゲーションバーのリンクが更新されないので、別ページに遷移してからプラクティスページのリンクをクリックするか、リンクを2回クリックするとスクロール位置が更新されます。
Screenshot
Mac OS XカテゴリのOS X Mountain Lionをクリーンインストールするを修了すると、
次回からのプラクティスページ一覧に遷移したときは次のUNIXカテゴリに遷移された状態で表示されます。
Summary by CodeRabbit
リファクタリング
テスト