Rubyアンチパターン: なんでもprivateメソッド
「大きなメソッドは細かく分割しよう」「外部から使わないメソッドはpublicにしない方がいい」といったコーディングのアドバイスに従って、とりあえずprivateメソッドを増やしていった場合に陥りがちなパターンです。
確かに巨大なメソッドばかりのクラスに比べると、メソッドは適宜分割されている方がマシですが、ここで安心するべきではありません。そのうち収拾がつかなくなります。
典型的な症状
- クラスのソース行が長く、メソッドが多い
- privateメソッドの方がpublicメソッドよりずっと多い
- privateメソッドの粒度や処理内容がバラバラで追いかけにくい
- 似たような名前のprivateメソッドが並んでいる
生じる問題
可読性が低くなる
いろんな責務が混在しているため、メソッド間の依存関係がわかりにくくなります。
テストが困難になる
内部ロジックにアクセスする手段がなくなるため、無理やりprivateメソッドを呼び出すか、テストの粒度が粗い結合テストでしのぐことになります。
再利用性しづらい
似たような機能を別のところで使いたくなっても、コードをコピー＆ペーストせざるをえなくなります。
そもそも対象クラス内でも似たようなコードが頻発したりします。
変更しづらい
どこかの機能を修正すると、その影響がどこまで及ぶかわかりにくくなります。
問題が生じる原因
privateは言うほどprivateではない
何より 同一クラスにあるprivateメソッド同士は自由に依存しあえる というのが大きいです。もちろん publicメソッドからも依存し放題です。その結果、privateメソッドが複雑さの温床になってしまうわけですが、privateの性質上これを防ぐことはできません。
つまり解決するべきことは「同一クラスにある」という点です。
解決策
クラスを分割する
複数の役割が混ざっているのであれば、分割するに越したことはありません。
そもそも「外部から使わないメソッドはpublicにしない方がいい」というのは、そのようなメソッドが対象となるクラスに存在しなければ問題ごと解消します。
であれば、単純にprivateにするのではなく、役割ごとにメソッドをまとめ、それを別クラスに抽出しておきます。そうすればメソッドが元のクラスからなくなるため、元のクラスの「外部から使われないメソッドがある」という問題も自動的に解決します。
例
ChatGPT先生に作ってもらったコードを手を入れたサンプルです。
改善前
レポートを生成するReportGeneratorクラスの例です。
外向けに提供したいのはgenerateメソッドのみなので、それ以外はprivateにしています。
が、いろんな責務が混ざっているため、何が何に依存しているのか分からないですし、そもそもReportGeneratorクラス内にロジックを書くべきなのかも怪しいです。
class ReportGenerator
def generate(data)
parsed_data = parse(data)
header = build_header(parsed_data)
body = build_body(parsed_data)
"#{header}\n#{body}"
end
private
def parse(data)
# 複雑な解析ロジック
end
def build_header(data)
# 複雑なフォーマットロジック
end
def build_body(data)
# 集計→フィルタ→マッピング
end
def sanitize_text(str)
# HTMLエスケープ
end
# ...
end
改善後
フォーマットするためのクラスReportFormatterと、dataを解析するためのクラスReportParserを導入し、それぞれにメソッドを振り分けます。
整理すると、ReportGeneratorにはprivateメソッドがいらなかった、ということになるかもしれません。
class ReportFormatter
def initialize(escaper:)
@escaper = escaper
end
def header(data)
# build_header 相当のロジック
end
def body(data)
# build_body 相当のロジック
end
private
def sanitize(text)
@escaper.escape(text)
end
# ...
end
class ReportParser
def parse(data)
# parseロジック
end
private
# ...
end
class ReportGenerator
def initialize(
formatter: ReportFormatter.new(escaper: HtmlEscaper.new),
parser: ReportParser.new
)
@formatter = formatter
@parser = parser
end
def generate(data)
parsed_data = @parser.parse(data)
header = @formatter.header(parsed_data)
body = @formatter.body(parsed_data)
"#{header}\n#{body}"
end
end
分割して抽出したクラスが専用の場合はネストさせてもよいでしょう。
class ReportGenerator
class Parser
# ...
end
class Formatter
# ...
end
# ...
end
参考にしたい知見
データ構造に注目する
上の例では、dataとparsed_dataは別物です。ということは、dataの詳細を扱うメソッドとparsed_dataの詳細を扱うメソッドは関係性が遠くなります。つまり同じクラスになくても構わない、ということです。
このように、扱うデータ・変数ごとにメソッドを分けて、それを反映したクラスを抽出するというのはよい習慣です。オブジェクト指向から離れた一般的な知見としていうと、「データ構造に着目し、役割に応じたデータ構造を導入するようにしよう」とも言えそうです。
ここからさらに敷衍すると、parsed_dataが複雑になってきた場合には、ParsedDataクラスの導入も検討しよう、ということでもあります。
Discussion