Rails: DHHのYouTube動画を辛口レビュー「On Writing Software Well #2」(翻訳)

概要 原著者の許諾を得て翻訳・公開いたします。 英語記事: 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 … Continue reading Rails: DHHのYouTube動画を辛口レビュー「On Writing Software Well #2」(翻訳)