Closed super-appy closed 8 months ago
リファクタリングは、コードをより整理し、効率的かつ読みやすくするプロセスです。RecipesController
のコードを見直すと、いくつかの改善点があります。具体的には、モデルへのロジックの移管、重複するコードの削減、およびリソースの効率的な取り扱いです。
make_taste_tag_time
メソッドは Recipe
モデルに関連する処理を行っているため、モデルに移管するのが適切です。
class Recipe < ApplicationRecord
# ...
def make_taste_tag_time(tag_ids)
formatted_tag_ids = tag_ids.map { |id| id.to_s.rjust(2, '0') }
str_tags = formatted_tag_ids.join('')
"#{taste}_#{str_tags}_#{time_required}"
end
end
これにより、RecipesController
からは以下のように呼び出せます。
@recipe.taste_tag_time = @recipe.make_taste_tag_time(recipe_params[:tag_ids])
new
アクションのリファクタリングnew
アクション内でのレシピの初期化をモデルに移管します。
class Recipe < ApplicationRecord
# ...
def self.build_with_ingredients_and_steps
recipe = new
3.times do
recipe.recipe_ingredients.build
recipe.recipe_steps.build
end
recipe
end
end
コントローラでは、次のように簡潔に記述できます。
def new
@recipe = Recipe.build_with_ingredients_and_steps
end
それぞれのリファクタリング案をコードで示します。
set_select_lists
メソッドのリファクタリングset_select_lists
メソッドで、Tag.all
の繰り返し呼び出しを避けるために、結果を変数にキャッシュします。
def set_select_lists
@all_tags = Tag.all
@tags = @all_tags.group_by(&:category).transform_values do |tags|
tags.map { |tag| [tag.name, tag.id] }
end
end
index
アクション内の @q.sorts
のリファクタリングRecipe
モデルにデフォルトの並び順を設定するスコープを追加します。
class Recipe < ApplicationRecord
# ...
scope :sorted_by_creation, -> { order(created_at: :desc) }
end
そして、コントローラの index
アクションでこのスコープを使用します。
def index
@tags = Tag.all
@q = Recipe.ransack(params[:q])
@recipes = @q.result.includes(:tags).sorted_by_creation.distinct
end
show
アクションでの @recipe_ingredients
と @recipe_steps
の取得Recipe
モデルに既に has_many
の関連付けがあると仮定して、show
アクションで直接これらの関連を利用します。
def show
@recipe = Recipe.find(params[:id])
@tags = @recipe.tags
unless @recipe.api_resp
@recipe_ingredients = @recipe.recipe_ingredients
@recipe_steps = @recipe.recipe_steps
end
end
これらの変更により、コントローラの複雑さが軽減され、コードがより整理されます。また、モデルに関連する処理はモデル内で完結し、MVCの原則に沿った構造になります。
リファクタリングは、コードの機能を変更せずに、可読性、保守性、拡張性を向上させるプロセスです。モデルにロジックを移管することで、コントローラーの責務を軽減し、MVCの原則に従うことができます。さらに、繰り返し使用されるコードの抽象化やメソッドの適切な配置により、コードベース全体の品質が向上します。
API用の RecipesController
をリファクタリングする際、重要なのは各メソッドの責務を明確にし、可読性を向上させることです。以下にリファクタリングの提案を示します。
set_select_lists
メソッドヘルパーメソッドを使用する ビューでの表示のためのデータ準備であれば、ヘルパーメソッドとして定義することもできます。これは、特にビューに関連するロジックの場合に有用です。
Copy code
module ApplicationHelper
def select_list_tags
Tag.all.group_by(&:category).transform_values do |tags|
tags.map { |tag| [tag.name, tag.id] }
end
end
end
この方法では、ビュー内で直接 select_list_tags ヘルパーを呼び出すことができます。
どちらの方法を選択するかは、アプリケーションの設計と、メソッドがどのように使用されるかによります。コントローラのロジックとして重要であればコンサーンを使用し、ビューの表示に主に関連する場合はヘルパーメソッドを使用するのが良いでしょう。
OpenAIクライアントの初期化は、繰り返し行われるべきではありません。これをモジュールまたはサービスオブジェクトに切り出すことができます。
module OpenAIService
def self.client
@client ||= OpenAI::Client.new
end
end
api
メソッドのリファクタリングAPIリクエストを行う api
メソッドは、より汎用的で再利用可能な形にリファクタリングできます。また、このメソッドはコントローラーに置くのではなく、専用のサービスオブジェクトに移動するのが良いでしょう。
class RecipeAPIService
def self.request_recipe(params)
response = OpenAIService.client.chat(
# ...
)
response["choices"][0]["message"]["content"]
end
end
data_change
メソッドのリファクタリングこのメソッドは、APIレスポンスを Recipe
モデルの属性に変換しています。この処理も Recipe
モデルまたは別のサービスオブジェクトに移動することができます。
class Recipe < ApplicationRecord
def self.create_from_api(response, params)
# api_resp_name, api_resp_ingredients, api_resp_steps を抽出してセットする
new(params.merge(
api_resp: response,
title: api_resp_name,
api_ingredients: api_resp_ingredients,
api_steps: api_resp_steps,
taste_tag_time: taste_tag_time
))
end
end
これらの変更に基づいて、Api::RecipesController
の create
アクションを以下のように変更できます。
def create
if api_request_allowed?
api_response = RecipeAPIService.request_recipe(recipe_params)
@recipe = Recipe.create_from_api(api_response, recipe_params)
if @recipe.save
set_api_request_cookie
redirect_to recipe_path(@recipe)
else
Rails.logger.info(@recipe.errors.full_messages)
set_select_lists
render :new, status: :unprocessable_entity
end
else
redirect_to root_path, success: 'レシピの生成は1日1度までです'
end
end
これにより、コントローラの複雑さが減り、各メソッドの責任が明確になります。また、再利用可能なコードの部分(OpenAIのAPIリクエストやレスポンスの処理)がサービスオブジェクトに集約されることで、将来的なメンテナンスや拡張が容易になります。
OpenAIだとエラー時の処理が難しかったので、Faradayのgemに変更!
Faradayでの実装の参考 https://zenn.dev/yoiyoicho/articles/20aca98dc0d045
set_select_lists
メソッドを Recipe
モデルに移管するのは適切ではありません。このメソッドは、ビューで選択リストを表示するために Tag
モデルからデータを取得して整形するものであり、直接 Recipe
モデルの責務ではないからです。
モデルは、データベースのテーブルに対応するデータとロジック(例えばバリデーション、アソシエーション、スコープ)を管理するのが主な役割です。一方で、set_select_lists
はビューで使用するデータを準備するためのメソッドであり、コントローラーの責務に近いです。
ただし、このメソッドが複数のコントローラで共有されている場合は、以下のようなアプローチを取ることができます。
Railsのコンサーン機能を使用して、共通のメソッドをモジュールとして定義し、必要なコントローラで共有することができます。
例えば、app/controllers/concerns/select_listable.rb
というファイルを作成し、その中に共通のメソッドを定義します。
module SelectListable
extend ActiveSupport::Concern
private
def set_select_lists
@tags = Tag.all.group_by(&:category).transform_values do |tags|
tags.map { |tag| [tag.name, tag.id] }
end
end
end
そして、このモジュールを必要なコントローラでインクルードします。
class RecipesController < ApplicationController
include SelectListable
# ...
end
module Api
class RecipesController < ApplicationController
include SelectListable
# ...
end
end
ビューでの表示のためのデータ準備であれば、ヘルパーメソッドとして定義することもできます。これは、特にビューに関連するロジックの場合に有用です。
module ApplicationHelper
def select_list_tags
Tag.all.group_by(&:category).transform_values do |tags|
tags.map { |tag| [tag.name, tag.id] }
end
end
end
この方法では、ビュー内で直接 select_list_tags
ヘルパーを呼び出すことができます。
どちらの方法を選択するかは、アプリケーションの設計と、メソッドがどのように使用されるかによります。コントローラのロジックとして重要であればコンサーンを使用し、ビューの表示に主に関連する場合はヘルパーメソッドを使用するのが良いでしょう。
はい、Railsでは1つのモデルに対して複数のコントローラを用意することができます。これは、特にアプリケーションが異なるコンテキストや役割で同じデータにアクセスする場合に役立ちます。例えば、管理者用のインターフェースと一般ユーザー用のインターフェースが異なる場合、それぞれに対応するコントローラを用意することが一般的です。
実装例
例えば、
Recipe
モデルに対して、一般ユーザー用のRecipesController
と、API用のApi::RecipesController
を作成することができます。通常のコントローラ(
RecipesController
)一般ユーザー向けのビューとロジックを扱います。
API用コントローラ(
Api::RecipesController
)名前空間を用いてAPI用のコントローラを作成します。これは主にAPIリクエストを扱います。
ルーティング
それぞれのコントローラに対応するルーティングも設定する必要があります。
このように設定することで、異なるコンテキスト(通常のウェブインターフェースとAPI)で同じモデル(この場合は
Recipe
)に対する操作を、異なるコントローラで処理できるようになります。これは、コードの整理と、異なるユーザーインターフェースやAPIエンドポイントの要件に対応するための良い方法です。