Ruby学習の一環として、週に1度Ruby関連のブログ記事(英語)を翻訳しています。
今回はメソッドからクラスを切り出す話。
原文：Extracting a class from a method
↓以下本文↓
他人が実際に使っているコードを見るのって勉強になりますよね。というわけで今回も僕が実際に作っているアプリのコードを見ていきたいと思います。
やること
5年分のデータを参照して、gem PRAWNを使ってPDFを生成するクラスがあります。それと構造は似ているけれども、1年分のデータだけを参照するクラスを作ることになりました。
どうやったか
まず、既存のクラスを見て、新しいレポートを作る上でどういう変更が必要になりそうか確認しました。どちらのレポートも非常によく似ていたので、クラスないの変更に留められないか、できるだけ大部分のコードを再利用できないかを考えました。
新しいレポート作成には、新しいクラスを作る必要があるとすぐに気づきました(別々の責務など)。それに、既存のPDFクラスをリファクタリングする必要もありました。コードは773行にもわたっていて、簡単には再利用できそうになかったのです。
まず最初に、既存のPDFクラスを小さいクラスに分割して、運用/テスト/再利用をしやすくすることにしました。
以下はその変更点の例です。このメソッドは既存のPDFクラスのもので、役割は一つ、データをグルーピングしてレポートの構造に落とし込んでいるようでした。このメソッドをクラスとして切り出すのが良さそうです。より小さなメソッドに切り出すのが良さそうな箇所も多くありました。最終的には、この新しいクラスは新しいレポートから呼ばれるようになります。
元々のメソッド
これは、773行からなるメソッド(コメントを含む)の一部です。注：記事を長くしすぎないためにコメントは一部省略し、名前や用語は変えてあります。
##
# Get all todos and group them by year for PDF creation.
#
# * *Args* :
# - +project+ -> {Project} project object.
# * *Returns* :
# - Hash of todos for given project grouped by year. Years are the keys for array of todo objects.
# * *Raises* :
# - ++ ->
def get_todos_grouped_by_year(project)
todos = project.todos.select('todos.*, meetings.meeting_date, groups.acronym, group_sub_types.acronym as group_sub_type_acronym').joins("left join group_meetings on todos.id = group_meetings.todo_id left join meetings on meetings.id = group_meetings.meeting_id left join group_sub_types on group_sub_types.id = group_meetings.group_sub_type_id left join groups on groups.id = meetings.committee_id").joins(:todo_type).where("(group_types.name = 'Content' and (groups.acronym = 'TIL') and (group_sub_types.acronym = 'POL' or group_sub_types.acronym = 'INFO')) or (group_types.name = 'Strategy') or (group_types.name = 'Content' and todos.actual_end_date > '2017-06-30' and meetings.meeting_date IS NULL)")
additional_todos = project.todos.select('todos.*, MAX(meetings.meeting_date) as meeting_date, groups.acronym, group_sub_types.acronym as group_sub_type_acronym').joins("left join group_meetings on todos.id = group_meetings.todo_id left join meetings on meetings.id = group_meetings.meeting_id left join group_sub_types on group_sub_types.id = group_meetings.group_sub_type_id left join groups on groups.id = meetings.committee_id").joins(:todo_type).where("(group_types.name = 'Content' and todos.additional = true)").group('todos.id')
todos += additional_todos
group_of_todos_by_year = todos.group_by do |element|
if element[:meeting_date].try(:year)
if (element[:acronym] == 'TIL') || element[:additional]
# Group by fiscal date.
year = (element[:meeting_date].month < 4 ? element[:meeting_date].year-1 : element[:meeting_date].year)
element.date_to_use = element[:meeting_date]
year
else
# Mark todos for deletion by setting key as delete.
'delete'
end
else
year = (element[:actual_end_date].month < 4 ? element[:actual_end_date].year-1 : element[:actual_end_date].year)
element.date_to_use = element[:actual_end_date]
year
end
end
# Get rid of todos marked for deletion.
group_of_todos_by_year.reject!{|k| k == "delete"}
# Ensure each todo is show limited times.
group_of_todos_by_year.each do |key, value|
group_of_todos_by_year[key] = value.group_by(&:id)
.map { |_k, v| v.delete_if { |x| (v.any? { |s| s[:acronym] == 'TIL' } && x[:acronym] != 'TIL') } }
.delete_if(&:blank?)
.flatten(1)
end
group_of_todos_by_year
end
クラスとしてのメソッド
新しく作ったクラスは、上記の長ったらしいメソッドがやっていた処理を全てするようにしました。
基本的な手順：
- まずは上記のメソッドをinitialize(project)をもつクラスに切り出し、処理は全てget_todos_grouped_by_year(project)という一つのメソッドに集約
- そのメソッドからさらに新しいメソッドを切り出す
- ネストが深くなっている箇所は分解して新しいメソッドに切り出す
- 処理がわかりにくい箇所も切り出す
- ユーザーがこのクラスのインスタンスを生成できるようにし、callだけで結果を得られるようにした(例：todos = ToDosGroupedByYear.new(@project).call)
- メソッド名を変更し、よりわかりやすく＆コメント削減
##
# Get todos for the PDF Report and group them by year.
class ToDosGroupedByYear
def initialize(project)
@project = project
end
##
# Get all todos and group them by year for PDF creation.
#
# * *Args* :
# - ++ -> {}
# * *Returns* :
# - Hash of todos for given project grouped by year. Years are the keys for array of todo objects.
# * *Raises* :
# - ++ ->
def call
final_hash = {}
group_year.each do |key, value|
final_hash[key] = value(value)
end
final_hash
end
##
# Return refined array of todos for year (each todo is shown limited times).
def value(value)
value.group_by(&:id)
.map { |_k, v| v.delete_if { |x| (v.any? { |s| s[:acronym] == 'TIL' } && x[:acronym] != 'TIL') } }
.delete_if(&:blank?)
.flatten(1)
end
##
# Return ALL default todos and the extras not included in the defaults
# already.
def combined_todos
# Queries are now broken out into their own class.
todos_query = ToDosQuery.new(@project.todos)
defaults = todos_query.defaults
todos_query.defaults + (todos_query.additionally_flagged - defaults)
end
##
# Return fiscal year.
def year(date)
date.month < 4 ? date.year - 1 : date.year
end
##
# Get rid of todos marked for deletion via their hash key.
def remove_todos(grouped_todos)
grouped_todos.reject { |k| k == 'delete' }
end
##
# Returns string to group todo by (year or 'delete').
def mark_printable_meetings(element)
if (element[:acronym] == 'TIL') || element[:additional]
# Group by fiscal date.
element.date_to_use = element[:meeting_date]
year(element[:meeting_date])
else
# Mark items for deletion by setting key as delete.
'delete'
end
end
##
# Group todos by meeting date or actual end date if no meeting date.
def group_year
grouped = combined_todos.group_by do |element|
if element[:meeting_date].try(:year)
mark_printable_meetings(element)
else
element.date_to_use = element[:actual_end_date]
year(element[:actual_end_date])
end
end
remove_todos(grouped)
end
end
なぜ後者の方がいいのか
このコードはまだまだ改善していきますが、とりあえず大きな利点は以下の通りです。
- 元のPDFクラスはこれよりも1つ機能が少なく、PDFを生成することだけを考えていた
- 新クラスToDosGroupedByYearはクラス名通りの動きをする
- 全体的にクラスが小さくなった
- 新クラスはテストができる
- 新クラスのメソッドは短めなので、読むのも運用も簡単
- クラスの再利用ができる/しやすい
- 細かく分けることで、リファクタリングが必要な箇所や危うい(wonky)箇所が見つけやすい