概要
原著者の許諾を得て翻訳・公開いたします。
- 英語記事: On Writing Software Well #2: Using callbacks to manage auxiliary complexity: A review - Ryan Bigg
- 原文公開日: 2018/03/26
- 著者: Ryan Bigg -- Ryan Bigg氏はRailsのcontributorであると同時にCulture Amp社のメンバーであり、RubyやElixirでの開発を行っています。RailsガイドのドキュメントやMultitenancy with Rails など多くの執筆実績があります。
お題の動画です↓。
Rails: DHHのYouTube動画を辛口レビュー「On Writing Software Well #2」（翻訳）
DHHの最近の動画シリーズについて思うところがあるんじゃないのかと何人かの方に尋ねられました。他にも、この動画に対する批判的な意見がないのが残念という声もありました。そこで、DHHの2本目の動画を題材に、私から辛口の意見を述べたいと思います。
シリーズ1本目の動画にしなかった理由は、この回が実によかったからです。コードがそうなるに至った理由を説き明かすコメント、これはコードベースのそうした部分に慣れ親しんでいない人にとって計り知れないほど有用です。DHHの説明も周到に練り上げられていて、私がツッコめる部分はどこにも見当たりませんでした（ここダジャレ！）。
訳注: commentingが「コメントを追加する」と「論評を加える」の両方の意味にかけているようです。
動画の第2回のキーワードは「コールバック」です。DHHがコールバックについてどんなことを語るのか、いたく興味を惹かれました。
以下の論評には、動画を3回見直したときの走り書きも含まれています。こういう本音ポロン出しを気に入っていただけた方は、ぜひ（原文）末尾のコメントにてお知らせください。
第一印象
- コールバック来た。もうたくさん。コールバックって、欲しくもないときに発生すること多いし。単体テストでモデルの
createを呼ぶときとかに、テストに全然関係ない振る舞いをコールバックが引き起こすし。びっくりさせるのは好きじゃないんで、この場合自分なら明示的に書くかな。 -
DHHの動画ではRailsのconcern（includeされてクラスに振る舞いを追加するモジュールのこと）の話題を扱うかもよという噂は小耳に挟んでたけど、もしそんな話を始めたら速攻で動画を一時停止してお気に入りの酒を持ってきてから、Railsアプリで使われてるconcernの話をするたびに一杯ずつひっかけるか。
1:35 「副作用」
- 「副作用、最近は評判が今ひとつだけどね、特に関数型プログラミング方面で」そりゃそうでしょう。メソッドを呼んで「魔法のようなクソがランダムに発生」すれば予測は難しくなるし。コードが何をしているかを明示的に書いておけば、「今」だろうが「将来」だろうが理解に苦しまないで済むし。DHHにはこの「将来」という視点が欠けているし。
2:13 「Messagesコントローラ」
@bucket.recordの引数、いくらなんでも多すぎ。こんだけ引数使って何をしようと？key/valueごとに改行ぐらいしないと
@bucket.record(new_message,
parent: @parent_recording,
status: status_param,
subscribers: find_subscribers,
category: find_category
)
この方がまだマシだと思います。しかもキーが追加されたり削除されたりしたときでもGitのdiffが見やすくなります。こうしてみると、categoryは最近追加されせいでこんな末席にポイッと置かれたように私には見えます。
正直少しばかり驚いたのは、ここで@parent_recordingをbefore_actionとしてセットアップしているにもかかわらず、find_subscribersやfind_categoryはそうではなく、明示的に呼び出しているのです。こういうのは一貫させて欲しいところですが、DHHがこのようにしたのは何か理由があるのでしょうか。これらのメソッドはコントローラの末尾にまとめられているのですが、この使い分けがよい選択なのかどうかを判断するうまいアイデアが思いつきません。
4:15 Mentionモデル
- このモデルはなかなかいい。
after_createやafter_commitのあたりでちょっと寒気がしたけど（オレはPTSDかと）。 - このモデルはなぜか
ApplicationModelを継承していないのが目を引く。たぶんレガシーアプリか何かだろう。 callsign_matches?のcasecmpの使い方は( ・∀・)ｲｲ!!after_commit :deliverのunlessは少々イラッと来たけど、すべてのロジックをメソッド内に閉じ込めておくのが好きなのかも。自分ならこう書くかな。
def deliver
return unless mentioner == mentioned
...
end
5:55 Recording::Mentions concern
- 飲む。concernだし。"concerning"（気がかりな、関連する、悩ましい）だからconcernだなんて笑えないジョークだし。
コールバック
- コールバックをさらに投入してるし。
remember_to_eavesdropはおそらくコントローラのメソッドチェインでさらに何かを設定するんだろうし。コントローラは変更があったかどうかをチェックして、そこからメンションを送信することを決定するんだろうし。
訳注: eavesdrop: 盗み聞きする、盗聴する
Current Attributes
もうひとつ気になるのはCurrentです。画面の下の方にちらっと見えるeavesdrop_for_mentionsで使われているやつです。
Currentについては既に「RailsのCurrentAttributesは有害である」で書きました。アプリのありとあらゆる場所で魔法のように使えるグローバル変数です。Current.userはどこで設定されるのでしょうか。そこに値があることをどうやって信じたらよいのでしょうか。
ジョブをトリガするロジックを抽象化する
私には、このeavesdrop関連全体が別の何らかのロジックでラップされているように感じられます（おそらくコントローラででしょう）。コードが実際的にそうしたラッパーを欲しがっています。おそらくBasecampとしては、テスト中にMessagesをデータベース内に永続化したり、このジョブコードを毎度毎度実行したりすることを望んでいないのでしょう。その代わりに、まさにこの場所でそれを行うことになります。これらの副作用でテストの速度が落ちそうです。
こういうのはService Objectとして抽象化して、そこでこのrecordingを作成してからこのEavesdroppingJobをトリガする方がよさそうです。これならcurrent_userをコントローラから渡せるというボーナスも付きますし、しかもですよ、CurrentAttributesグローバル変数などという代物も取っ払えるのです。
コードで表せばたぶんこんな感じです。
module Mention
class EavesdropForMentions
attr_reader :recording, :params, user
def initialize(recording:, params:, user: )
@recording = recording
@params = params
@user = user
end
def run
return unless eavesdropping?
Mention::EavesdroppingJob.perform_later recording, mentioner: user
end
private
def eavesdropping?
(active_or_archived_recordable_changed? || draft_became_active?) &&
!Mention::Eavesdropper.suppressed? &&
recording.has_mentions?
end
def active_or_archived_recordable_changed?
# recordable + paramsを用いて変更をチェックするコード
end
def draft_became_active?
# recordable + paramsを用いて変更をチェックするコード
end
end
実際にBasecampの巨大アプリをいじったわけではないので、このコードが動くかどうかまではわかりません。わかっているのは、このコードは後でMention::EavesdroppingJobを実行する可能性のあるコードを見事にカプセル化していることと、Recordingを保存するといつ何どき副作用としてジョブがキューイングされるかもしれないという問題を回避できるということです。私のアプローチではこの2つを切り離したことで、それぞれが独立して発火できるようになっています。
本質的には（おそらく）コードをどっさり書けば同じことはできます。しかしモデルの保存を切り離せたのは自分の中では大勝利です。
Callbackを抑制する
さらなるグローバルステート（Mention::Eavesdropper.suppressed?）が出し抜けに現れました。これはどんな問題を引き起こす可能性があるでしょうか。果たしてこれがどの場所でオンオフされるのかを、このメソッドに到達するコードパスから追えるでしょうか。デバッグがつらそうな予感がします。
DHH自身はこれについて、ここはコールバックが発生して欲しくない場所かもしれないと考えているそうです（動画の11分目ぐらいまで）。そうですか。ならば上述のようにこれをコードのオプション部分とし、Mention::Eavesdropper.suppressed?などという「不気味な遠隔操作」はやめにすることです。
訳注: spookey-action-at-a-distanceはGeorge Musserの書籍のタイトルのもじりで、本来は量子力学におけるquantum entanglement（量子もつれ）という遠隔作用を表します。
コードを再編成して、このeavesdrop的振る舞いのトリガをオプション化すれば、このコードに取り組むときに余分な認知能力を使わずに済むのではないでしょうか。
has_mentions?
特記しておきたいのは（うふっ☺）モジュールの末尾にあるhas_mentions?です。これは少なくとも、何らかのメンションが行われたかどうかをチェックするという振る舞いを抽象化するようです。
訳注: special mentionの「mention」が「言及する」という意味であることにかけたシャレのようです。
13:23 Mention::EavesdroppingJob
ここで使っているCurrent.setは、Mention::Eavesdropperやこれに関連するものにアカウントを渡す方法としては相当しょぼい気がします。Eavesdropperクラス内でアカウントにアクセス可能だとしたら（以下のコードのように設定されていると仮定）、こんなふうにラップした理由が私には見えません。
class Mention
class Eavesdropper
attr_reader :recording
def initialize(recording)
@recording = recording
end
...
end
end
しかし（私の）このEavesdropperは少なくとも抽象化がなされていますし、concernではありません。concernはこのアプリではちょっとした「黄金のハンマー」的に使われているように感じられます。
13:45 全体をざっと眺める
DHHはコントローラーからRecording::Mentionsというconcernに画面を移動しました（飲みましたとも）。
第一印象としては、仮にこのアプリに不慣れな開発者がeavesdrop的振る舞いをデバッグすることがあれば、このRecording::Mentionsなるconcernの中でその振る舞いを見つけられると推測するだろうと思いました。
この種の「不気味な遠隔操作」や、私が数年前に書いてしまったこの手のコードのような代物は、書いたその時点ではとても冴えているように思えても、数か月後にもう一度見直してみれば「おいらは一体何をしようとしてたんだっけ？」となるのがオチです。
DHHは14:10のあたりで「間接的な操作がだいぶ増えたけれど、そのおかげでこのメソッドで何を行っているかという流れがとてもスッキリ見えるようになった」と語っています。私がこの動画でダントツに賛成できないのがこの発言です。この流れは「不慣れな」私の目にはまったくもって曖昧以外の何物でもありません。私はこのアプリの事情など知らないのですから。
このコードは「賢い」と同時に危険を孕んでいます。未来の自分が数か月後に再びこのコードを目にすれば、果たして今度もこのやり方で完璧にやっていけるだろうかと首をかしげるでしょう。
14:20 Mention::Eavesdropper
ここで私たちは、「単一責任の原則」という考えがいかに大事であるかを実際に体現したクラスを目にすることができます。
（興味深いことに、DHHは14:30までのあたりでコードを間違ってたどっています）
CurrentAttributesについては既に別記事で書いたので繰り返しませんが、やはりここで長い長い溜息が漏れてしまいます。グローバルステートというものがありとあらゆるアプリから消え去ってしまえばいいのにと思わずにいられません。
「グローバルな要素は、アプリからなくしてしまえばいいというものではないのです」勘弁して！まさかそれを実演するとは。「そういったものをたらい回ししたところで有用になるものでもない」この動画で2番目に賛成できない発言です。たらい回しすれば、それらがどこから来たのかということを自覚できますし、チェインを遡れば元々の定義にたどり着けるかもしれません。このCurrent.personなる謎の代物は、十分な理由もなしにあらゆるものを隠蔽してしまっています。
私の今の気持ちを余すところなく表すのはもうこの言葉ぐらいしかないかもしれません。
「グローバルステートよ、天に召されるがよいっ」と。
16:24 Mention::Scanner
Mention::Scannerのアプローチは割りとまともそうでほっとしました。このコードではEavesdropperに単に丸投げしていないのがよい点です。Mention::Scannerは何らかの意味で関連があるからです。これは独立したconcernであり、このロジックをMention::Scannerに移動するのはよいアプローチです。
18:10 Mentionのafter_commitフック
これは、単純にコントローラで@bucket.record呼び出しの後に呼び出せるのにと強く思います。
19:20 ProjectCopierと、suppressによる抑制
事前に「events」や「deliveries」を明示的に作成するオプションがこのコードにあれば、suppress_events_and_deliveriesの裏側の抑制用チェインは不要になるのではないでしょうか。やはりこれも残念な仕事だと感じてしまいます。こんなことをするぐらいなら、30分ほど頑張ってきちんと整ったコードを書けばよいのにと思います。
さらに、20:10あたりまでのDHHは、抑制可能な振る舞いがどこから来ているのかを見つけられずにいます。追うのがつらいコードには、並々ならぬ「コードの臭い」が潜んでいます。繰り返しますが、仮にこれらの「コールバック」が暗黙的ではなく明示的なものであれば、抑制はそもそも不要になるのではないでしょうか。それならばコードのフットプリントも縮小されて明快さも増し、ひいては理解しやすくなるのではないでしょうか。「このコードはAとBとCを行う」、そこにはマジックの介在する余地などありません。
20:39 まとめ
「これでスッキリしたんじゃないかな」いえいえとんでもない。私もRailsを10年やってますし、コードベース内でこのコードを見かけたら、より明示的な書き方、もっと取り組む値打ちのある書き方を模索することでしょう。
私にはこのコードが、先ほども申し上げた「賢いコード」のように感じられます。「見て見て、Rubyのスゴい機能を全部使ってるの、ボク賢いでしょ！」という具合です。はいはい、賢いですね☺。
しかし何か月かが過ぎてすべてが完璧に動いた頃に、バグを踏むでしょう。そしてコードを見返して「一体どうやったら収まりがつくんだ」と頭を抱えることでしょう。マジックの使いすぎにもほどがあります。
しかしこれが「Rails Way™」というものなのでしょう。
20:46:「このロジックをまとめて詰め込む先は...さてコントローラかな？Service Objectかな？」そこではありません。トランザクションオブジェクトです（上述の5:55の私のコード例を参照）。recordingを作成し、かつ、Mention::Scannerやdeliveriesを明示的にトリガするトランザクションオブジェクトです。
別のアプローチとしては、dry-transactionを使う方法も考えられます。このgemのDSLはとてもよくできていて、こういうのを設定するのに向いています。これを使えば、こんなふうに書けるでしょう。
class CreateRecording
step :create
step :scan_for_mentions
step :deliver_notifications
def create(bucket: bucket, ...)
# @bucket.recordのコードはここ
end
def scan_for_mentions
# MentionScannerはここ
end
def deliver_notifications
# Deliveryコードはここ
end
何もかもが1つのクラスにきれいに収まっています。置くべき場所はこっちです。コントローラでもService Objectでもありません。しかも、このトランザクションに関わる手順がきちんと記述されているトランザクションオブジェクトには、マジックのかけらも見当たりません。手順は書いた順序のとおりに実行されますし、どの手順でもトランザクションを中断できます。
私はこのアプローチが好みです。元のコードベースではコールバックとその暗黙的な手法がこうした害をもたらしていたのです。私がその手法に手を出すことはまずありません。私なら、操作の順序が明確に示されるトランザクションオブジェクトを使うでしょう。