Rails 開発で意識していること 2

Rails 開発で意識していること 1 の続きです。

前回は曖昧にしていましたが、ここでモデルに関する単語の使い分けを決めておきます。

  • ActiveRecord - ORM & Repository 機能を実現する Rails の抽象クラス (ActiveRecord::Base)
  • Active Record - ActiveRecordを継承し RDB のテーブルにマッピングされるクラス
  • モデル - MVC の M

ビジネスロジックをどう構築するか

前節で書いたプレーンなクラスを基礎とするモジュール化は、ビジネスロジックをユースケースごとに実装するのに使えそうです。 しかしながら、素の Rails にそのようなモジュールを置くためのレイヤーはありません。 Rails がベースとしている MVC の区分でいえばモデルの部分がそうなるはずですが、 Rails は基本的に「モデル == Active Record」と位置づけているように見えます。 Rails Guides (v5.2) の Active Record Basics を見ても以下のようにあります:

Active Record is the M in MVC - the model - which is the layer of the system responsible for representing business data and logic. Active Record facilitates the creation and use of business objects whose data requires persistent storage to a database.

Active Record は MVC におけるモデルにあたり、データの永続化を伴うビジネスロジックを表現する、とあります (まあこれは Active Record とはモデルだと言ってるだけでその逆ではないのですが)。 なので Active Record のパターンに当てはまるロジックなら問題ないものの、そうではないケースはどうするのか、というのが Rails ではよく問題になります。 例えば、

  • 1つのユースケースが複数の Active Record に関わる場合
  • 1つの Active Record が複数のユースケースに関わる場合
  • 外部サービスとの連携など永続化以外の副作用を伴う場合

などです。

この辺をハンドルするための仕組みが用意されていないのは Rails の弱点と言えるかもしれません。 しかしながら、これは意図的に選択された方針ゆえの制約なのだと思います。 以下のスライドにすごく良くまとまっているのですが、このスライドの言葉を借りると Rails は色んなものを密結合にして諸々の Easy さを実現しているフレームワークなので、ある程度の規模になると必要に応じてその方針に逆らいながらコードを構築する必要が出てきます。

Ruby on Rails の正体と向き合い方 - Speaker Deck

とはいえ先ほど触れた Rails Guides にしても、Modelsセクションの最後にはActiveModelを紹介する Active Model Basics という章もあります。 ActiveModelは、 Active Record ライクなモデル定義の仕組みを ORM とは切り離して使えるライブラリです。 なので、必ずしも「Active Record でないオブジェクトをモデルとすべからず」と言っているわけではありません。

というわけで、 Active Record に収まらないビジネスロジックのまとめ方については大きく以下の2方向がありうると思います:

  • モデル層を拡張する
    • Active Record かどうかにはこだわらず、ドメインモデルを積極的に抽出してロジックの核とする。
  • モデル層の上に新たなレイヤーを設ける
    • ユースケースを扱うレイヤーを作りそこにロジックを置く。 Active Record はロジックの構成要素の1つになる。

後者でいう新たなレイヤーはよくサービスと名付けられます。 これらは別に二者択一ではなく、要件に応じて適したやり方を都度考える事は出来るはずです。 ただ後者の方向に強く傾くと、モデルとは Active Record の事だと割り切り、モデル層はなるべく薄くしよう (ただのエンティティ的に扱おう) という風にもなります。
実は自分は割とそうなりがちで、ユースケースはひとまずトランザクションスクリプト的にサービスとして実装してしまいます。 これはある種の設計をサボっているとも言えるかもしれません。 これがベースになるとapp/models内には (抽象クラスを除き) エンティティ的な Active Record (+ Validation, Callback) だけが置かれ、ビジネスロジックは全部app/servicesに入ってるという感じになります。 雑かもしれませんがこれはこれでわかりやすく、最低限サービスの責務の切り分けがちゃんと出来ていれば問題なく運用できる印象です。 なのでこれまでは基本この方法で良いだろうと正直思ってたのですが、この機にドメインモデル貧血症の話や設計から逃げるためにサービスクラスを使うなという記事を改めて読み返してみると、改善すべき点もあるかもしれないと思えてきています。
例えばActiveRecordcomposed_ofという機能があるのも、恥ずかしながらつい最近まで知りませんでした。 これを使えば独自の値オブジェクトによるモデルの構築がしやすそうですね。 この記事を書いていて、モデルの設計について再考してみたいと思うようになりました。

何にせよ Rails におけるモデルやビジネスロジックの考え方・切り分け方については、実は明確に語れない状態だったという事に気づきました。
とはいえ大事なのは「モデルかサービスか」のカテゴリ分けを頑張る事ではなく、責務や変更可能性に応じてコードを適切にグルーピングし依存を管理する点だと思うので、ひとまずは現時点で気をつけるべきだと思っている点を引き続き書いていきます。

モデルは Entity + Repository に専念?

Active Record なモデルは DB のテーブルと (基本) 1:1 にひも付き、そのクラスオブジェクトは DB アクセスを行うリポジトリとして、インスタンスはレコードにマッピングされるエンティティとして振る舞います。 つまりは既に充分な責務を負ってるので、それ以上の責務はあんまり追加しない方が良いんじゃないかと思ってます。 ただ前述のようにそもそも Active Record じゃないモデルについてはちゃんと考えてこなかったので、タイトルにはをつけてます。

とりあえず Active Record には以下だけを持たせる事が多いです:

  • has_manyなどの関連定義
  • enum の定義
  • バリデーション
  • 使い回しやすいスコープ
  • 便利なアクセサメソッド

最後のやつは、例えば請求書の明細を表すモデルがあったとして、

  • 明細は商品の値段 (amount) と個数 (count) を持っている
  • 明細の合計金額を計算したい

といった場合に作られるであろう以下のようなメソッドです:

def total_amount
  amount * count
end

更にこれら明細をまとめる請求書モデルなら、全明細の合計を表すtotal_amountを持つかもしれません。

def total_amount
  invoice_items.sum(&:total_amount)
end

こういうメソッドがあれば、モデルの使い手がいちいち合計金額の算出方法を知らずにすみます。

しかし、では例えばこういった請求の明細内容を「確定する」処理なんかはどうでしょうか?
単にレコードの状態を変えるだけで済むならupdate(fixed_at: Time.current)くらいだし、請求書モデルがそういうインスタンスメソッドを持っていてもいいかもしれません。 しかし実際には通知の送信や関連するデータあるいはキャッシュの更新など、それ以外の副作用を伴うケースもよくあると思います。 その場合にこの確定処理をモデルに持たせるなら、一連の処理をセットにしたメソッドを作るか、ActiveRecordのコールバックで副作用を実行するかになると思います。 が、個人的にはどちらもあまり選択したくありません。 Active Record だとクラスのコンストラクタを使って依存を注入するやり方は使えないし、ActiveRecordのコールバックは他のユースケースと競合しやすく、経験的には時間が経つにつれて「特定の場合は除きたい・実行したい」という条件分岐で継ぎ接ぎされがちだからです。 のみならず、テストで Active Record のインスタンスをダミーデータ的に作って使う際に、自動で走る各種コールバックが邪魔になる事もあります。 よって、モデルの状態変更以外を伴うこの手の処理は、例えロジックとしては複雑じゃなくてもサービスとして切り出す方が無難じゃないかと思っています。
正直この辺の「モデルにどこまで持たせるか」に明確な基準は見い出せていないのですが、 とりあえずは「モデルに持たせるとテストしづらいなら切り出す」くらいで良いかもしれません。

またそれとは別にモデル以前の問題として、RDB のテーブル設計をサボらない事も大前提でしょう。 データはアプリの基盤ですから、可能な限り整合性を保ちやすい形で保存すべきモノ・コトをテーブル化し、 RDB の制約を活用してデータを守っていく事は、コードの設計にも増して重要です。

ユースケースは個別にモジュール化する

次に、モデルとコントローラの間に位置するサービス層について考えます。
こういうレイヤーの必要性を感じるのは、あるモデル (Active Record) に関する処理が、何でもかんでもそのモデルクラスに詰め込まれているようなコードを見た時です。 これは本来なら分けるべき Active Record が1つのクラスになっているというケースもありますが、先に載せた Ruby on Rails の正体と向き合い方 にもあるように、1つのモデルがいつの間にか複数のユースケースを抱えている場合も多いと感じます。 例えば同じデータでも、登録方法に Web 画面経由と CSV のアップロードによる一括登録の2種類があったり、 ページや機能ごとに条件や結果フォーマットの異なる検索処理が何種類か必要だったり。 そういった本来は別物である複数の処理が、同じ DB のテーブルを扱っているというだけの理由で1つのモデルクラスに詰め込まれると、無関係なメソッドが同居したり、バリデーションやコールバックが条件分岐だらけになったりします。

これは辛いので、自分はよくこの手の処理を個々の独立したクラスとして実装します。 しかし素の Rails にそういうレイヤーはないため、だいたいapp/servicesのようなディレクトリを作り (app/libの時もある)、そこに置いていきます。 でこれらのクラスを便宜的にサービスと呼んでます。 サービスはモデルや他のサービスに依存しつつ、1つ1つのユースケースを実現する役割を担います。 そしてコントローラは Web レイヤーとビジネスロジックを仲介する役割を担い、基本はサービスを呼び出して処理を移譲するだけ、という感じになります。 このようなビジネスロジックを担う層を設ける事で、フレームワークとロジックを分離しやすくなり、ロジック自体も個々のユースケースに沿って独立させやすくなります。

ロジックをサービス化する際には以下を意識します:

  • サービスの担う責務 (=テスト項目) を明確にする。
  • 1つの責務を複数のサービスに重複して負わせない。
  • 依存関係は実行時に差し替え可能にする。

これはひとえにテスタビリティとメンテナビリティを確保するためです。 ここに、前回の記事で記したようなプレーンなクラスによる実装がフィットすると考えています。 余計な仕組みを持たず、依存は全てコンストラクタで受け取るというシンプルな作りでサービスを構成出来るからです。

サービスの種類は大別すると登録・更新系 (書き込み) と検索系 (読み込み) に分かれると思いますが、特に役割に応じた分類などはせずapp/servicesに置いちゃいます。 サービスの中間データや結果の型が欲しい時は、だいたいサービス内にクラスを定義して使います。 あるいは複数の責務に分割可能な大きめの処理なら、ファサードとして1つサービスを作り、そのネームスペース内で更に小さなサービスを複数作った上で、ファサードがそれらに依存する形で全体の機能を実現したりもします。

そして今の所は上述した責務の切り分けなどを守ればひとまず OK で、それ以上のルールはあまり作らない事が多いです。 そのくらいゆるい方が、既に雑多なクラスやミックスインをapp/servicesに持つ既存プロジェクトをメンテしていく場合なんかは、少ない手間でビジネスロジックを守っていきやすい気がします。 しかし、ちゃんとやるなら例えば Hanami の Interactor みたいに各サービスはcallというメソッドだけを公開する、とか決める方が良さそうではあります。

コントローラはやっぱり薄くする

コントローラはリクエスト情報からモデルまで何でも触れてしまうのでついロジックを書いてしまいがちですが、これを許すとコントローラの責務が再現なく大きくなるし、コードの共有やテストもしづらいので避けたいところです。 Rails にはコントローラをテストする仕組みも用意されているものの、これはActionDispatch::IntegrationTestというクラス名にもある通り結合テスト寄りです。 テストの入出力はリクエスト・レスポンスの形式となり、依存のモック化も (自分が知る限りだと) 普通はしません。 そのため、コントローラにはそれ単体でテストできなくても問題ないコードのみを置くべきとなります。 すなわち DSL 記述に徹し、ロジックはサービスクラスに移譲するのが無難です。

アクションフィルタは最小限に

before_actionなどのフィルタは便利だし使い時はありますが、必要以上に使われているのをちらほら見かけます。 典型的には複数のアクションに共通で使う何らかのインスタンス変数をセットする処理です。

before_action :find_issue, except: %i[search]

def show
  # Use @issue...
end

def find_issue
  @issue = Issue.open_by(current_user).find(params[:id])
end

確かにコードの共通化は出来るものの、アクションがフィルタ内でセットされるインスタンス変数に暗黙的に依存する事になりコードの認知負荷を上げがちです。 こういうのは共通化するにしてもフィルタではなくただのメソッドとして行い、インスタンス変数へのセットは各アクションでやる方がわかりやすいです。 フィルタのonlyexceptをメンテする手間も省けます。

def show
  @issue = find_issue(params[:id])
  # ...
end

フィルタにはユーザ認証やロギングなど、広く必要とされアクションのコードに直接影響しない処理のみを置くのが良いと思います。

コントローラも単体テストしたい

ちょっと脱線になりますが、 Rails について考えた事の1つなので書いておきます。

前述のように Rails にはコントローラを単体テストする仕組みが用意されていないようなのですが、 個人的にはコントローラの単体テストがしたくなる事もあります。 例えば、処理の結果に応じて HTTP ステータスコードや HTTP ヘッダを返し分けたいとしましょう。 この時処理自体はサービスに移譲すれば済みますが、そのサービスの結果に応じてレスポンスの内容を変えるのはコントローラの責務とする方が自然です。 すると、結果に応じたレスポンスをちゃんと返すかのテストをさくっと置いときたくなります。 コントローラにモックのサービスを渡してテストできると楽なのですが、それをシンプルにやる方法は用意されていません。 HTTP まで意識したサービスを作ってそいつが直接ステータスコードなどを生成する手もありますが、そもそもコントローラ自体が単体テスト可能であればそういう苦労は不要なのになーと思います。 実際 Hanami の Controller (というか Action) はコンストラクタで依存を渡せるし、単体テストも可能なようです。
それ以外にもコントローラはたくさんの親クラスのメソッドに頼る必要があったり、結果 (ビューへのパラメータ) をインスタンス変数として表すのが基本だったりするのも、コードの可読性を下げがちで不満でした。 なので、その辺の不満を解消できる ActionHandler という gem を作った事があります。 これを使うとコントローラと同じ機能をハンドラとして以下のように書けるので、テストがしやすいです。 必要な情報は引数として宣言すればアクセスでき、ビューへのパラメータを含む結果は全て戻り値として表現します。

class UsersHandler # Like UsersController
  include ActionHandler::Equip

  def initialize(ranking: RankingService.new)
    @ranking = ranking
  end

  def index(params, cookies)
    puts "cookies: #{cookies}"

    users = User.order(:id).page(params[:page])
    ranks = @ranking.generate(users)

    render locals: { users: users, ranks: ranks }
  end
end
# test
handler = UserHandler.new(ranking: mock_ranking_service)
result = handler.index(mock_params, mock_cookies)
expect(result[:locals]).to eq(expected_result)

が、現状特に実運用はしていなくて、本来コントローラだってもっと副作用を局所化した形で書けるはずだという事を確かめるために作りました。
どうしても Rails のコントローラで依存を差し替えたい場合は、使用するサービスをインスタンス変数化しておき、テスト時にはそれにモックをセットする感じかなと思っていますが、あまりちゃんとやった事はないです。

DB マイグレーションは冪等に

DB の変更履歴をコードとして管理できるマイグレーション機能はとても便利です。 そしてマイグレーションはその性質上、可能な限り「いつ」「どこで」「何回」実行されても同じように動作する事が理想です。 例えばそのマイグレーションを作った当人の開発環境でしか動かない、作った時は動いたけど数ヶ月後に実行したらエラーになる、といったマイグレーションばかりでは、せっかくの便利さも半減します。 そうならないためにはいくつかの点に気をつけます。

参考: https://qiita.com/snaka/items/017cddd1a647d161cabd

マイグレーション内でモデルを使わない

マイグレーション内では基本的に DSL 以外の Ruby コードを使うべきではありません。 これは油断するとやってしまいがちですが、例えば以下です:

def change
  create_table :preferences do |t|
    t.integer :theme, null: false, default: Preferences.themes[:light]
    # ...
  end
end
class Preference < ApplicationRecord
  # ...
  enum theme: %i[light dark sepia]
  # ...
end

enum なカラムを持つpreferencesというテーブルを作成するマイグレーションで、そのカラムのデフォルト値をモデルの enum 定義を使って指定しています。 これは一見良いプラクティスのように思えます。 ここでいうlightなどの enum 値が、 DB 上で実際にどんな値にマッピングされるかを意識せずにデフォルト値を記述できるからです。
しかし、もし数年後にpreferencesテーブルが削除されたらどうなるでしょう? その場合もこのマイグレーションファイル自体は依然として残り続け、どこかで実行される可能性があります。 何らかの事情でテーブルの削除後にこのマイグレーションが実行されると、その時点のコードベースにはPreferenceクラスは存在しないはずですから、マイグレーションはエラーになってしまいます。 するとその原因調査や修正に余計な手間が発生する事になります。
まあ実際には新たに DB 環境を作る場合はdb:schema:loadでスキーマファイルを読み込むでしょうし、過去のマイグレーションを走らせるタイミングって実はそれ程無いとは思います。 しかしそれでもマイグレーションが仕組み的に将来のどこでも実行される可能性を持っている以上は、それを配慮した作り方にせざるを得ないと思います。
以上の理由で、自分がこの例のようなマイグレーションを書くなら、デフォルト値には素直に対応する数値を書きます。

t.integer :theme, null: false, default: 0

ただこういった配慮が必要になってしまう時点で、そもそもマイグレーションという履歴管理の仕方自体に問題があるとも考えられます。 実際、過去のマイグレーションがずっと残るのはコード管理の面でも不便な事があります。 例えばプロジェクトのコード規約が変わった時、過去のマイグレーションファイルにも適用すべきか、しないならリンタやフォーマッタの挙動をどうするか、ある時期より前のマイグレーションファイルだけ無視するような設定を入れるか、あるいはマイグレーションファイル全体を対象外としてしまうか…などと考える事が増えます。 そうなるくらいならマイグレーションという考え方自体を見直し、Ridgepole のように過去のマイグレーションファイルを必要としないやり方の方が良いのかもしれません。 まだ試してみた事はないのですが。

データの変更を含める時は慎重に

これも前節の話に似ていますが、安易にデータの追加・更新・削除をマイグレーションに含めてしまうと、実行する環境やタイミングによって結果が変わってしまうケースがあります。 他にも下記のような懸念点があるので、やるならそれらを加味した上で問題ないかを判断するのが良さそうです:

  • 決まった値のレコードを追加するようなケースだと、既存のデータ次第でユニーク制約や外部キー制約などの違反によりマイグレーションが失敗する可能性がある。
  • サーバへのデプロイ時に自動でマイグレーションを実行している場合は、データ更新に時間がかかるとデプロイ時間がのびてしまう。
  • db:schema:loadで DB を作る場合はデータ構造の定義のみが反映されるので、db:migrateがデータ操作まで含んでいると両者の実行結果にずれが出る。

互換性を保つマイグレーションを行う

これはマイグレーション単体ではなくコードベース全体の問題ですが、ロードバランシングを行っている本番環境などで複数のサーバ (正確には Rails アプリ) が同一の DB を参照している場合は注意が必要です。 マイグレーションによる DB の更新は一度だけ走るのに対し、サーバ上のソースコードの更新はサーバごとに行われる事になります。 するとサーバによっては「DB はマイグレーション実行後の状態だけどサーバ上のソースコードはマイグレーション実行前の状態」になる時間が生じうる事になるので、その間も問題なく動作するようなマイグレーションの仕方をする必要があります。

詳細: https://github.com/elafarge/blog-articles/blob/master/01-no-downtime-migrations/zero-downtime-database-migrations.md

とはいえ特に小・中規模のアプリだと、常にユーザがアクセスしているわけでもない機能の修正であれば、ほんの数分くらい互換性のない状態になっても問題ないケースは全然ありえます。 なので、必要に応じて意識すれば良さそうです。

テストを負債にしない

あとはテストです。テストは大事ですがただ書けば良いというわけではありません。

テストは落ちるためにある

例えば最初の記事の冒頭にフレームワークの DSL とビジネスロジックは分けて考えるべきだと述べましたが、これはテストにも言えます。 そこでも少し触れましたが、例として以下のようなモデルのバリデーション定義がある時:

class User < ApplicationRecord
  validates :name, presence: true
end

nameが空のUserは invalid になる」事を確かめるだけのテストは必要でしょうか?
自分は不要だと思います。この記述が期待通りに動作するかは Rails が責任を持つ部分だからです。 そのため、この手のテストはほとんど DSL が正しく書けているかのダブルチェック的な意味しか持たないはずです。 そういうダブルチェックが全く無用だとは思いませんが、やり出すとキリがなくなります。 Uservalidates記述を1つずつ確かめるようなテストケースを書いても、validatesの定義を変える度に修正されるだけのテストになるでしょう。 なのでこの例であれば、ユーザ登録などのユースケースをテストする過程でバリデーションが機能しているかを間接的に確認できれば充分ではないかと思います。

テストはバグを発見する事はできても、バグがない事を証明する事は出来ません。 なので上の例のように (仕様が変わらない限り) 落ちないテストの記述に時間を割くよりは、アプリのユースケースに沿ったテストを書き、

  • 将来のリファクタリングや機能改善で起きうるバグの混入を検知可能にする
  • そのモジュールの用例やエッジケースをコードとして可視化する

ためにこそテストを残すべきではないでしょうか。

実装ではなくインターフェイスをテストする

これはモジュール同士がインターフェイスにのみ依存すべきなのと同じ理由です。 実装の詳細をなぞるだけのテストは、実装をダブルチェックしているだけのようなものでしょう。 そうではなく入力と欲しい出力のペアだけを用意して、中身の実装に関わらず期待する結果が得られる事を担保しておく方が変更に強くてわかりやすいです。

その他の Tips

以上で大体は書けたので、後は細かな考えをメモしておきます。

RuboCop は大いに活用する

インデントなどのフォーマットが意味もなくバラバラなコードは読みづらいと思うので、自分はだいたい RuboCop を導入します。 ただ RuboCop のデフォルトルールには割とアグレッシブなものが多いせいか、あまり RuboCop を使いたくないという声もちらほら聞きます。 しかし設定は好きなようにカスタマイズ出来るし、基本的なフォーマットを自動で統一できるのはとても便利だと思うので、自分は不要だと思うルールは積極的に無効にしつつ使っています。
いつも使う代表的なルールをいくつか挙げると:

  • インデントやスペーシングの統一
  • クォートの統一
  • 末尾カンマの強制適用 or 排除
  • 未使用変数の発見

などがあります。 ただ「どこまで統一したいと思うか」は確かに人によりそうで、例えばn > 0n.positive?はどちらかに統一しようというStyle/NumericPredicateのようなルールもあるのですが、個人的にはそのくらいなら状況に応じてより読みやすそうな方を使えば良いように感じます。 しかしインデントの統一にはこだわるのになぜこちらにはこだわらないのかと言われるとあまりハッキリした答えはなく、ルールの数だけ bikeshed を引き起こしそうな面倒さはどうしてもあります。
チーム開発の場合は種類が違うこだわりを持つ人がいると衝突するかもしれませんが、私はとにかく統一されてれば良い派なので、ゆずれないこだわりを持つ人がいるならそれに合わせます。 逆に折り合いがつかなかったり、ルールの多さにストレスを感じる人もいるなら、全員が納得できるルールだけ使うのが良いでしょう。
またメトリクス系のルールは1行の最大文字数だけ使い、メソッドの行数や複雑度 (ABCサイズなど) などの上限を決める系のルールは使わないか、閾値をかなり高めに設定します。 デフォルトの閾値を超えるかどうかとコードの読みやすさは必ずしも相関しないと感じる事が割とあったからです。

テーブル定義のtimestampは前の方がいい?

ActiveRecord が自動でセットしてくれるcreated_at, updated_atというタイムスタンプカラムは、大体一番後ろのカラムとして定義されると思います。

create_table :users do |t|
  t.string :name, null: false
  t.integer :age, null: false

  t.timestamps
end

ただ、最近これらのカラムは先頭に置いとく方が便利な気がしています。 その方が将来カラムを追加したくなった時に、カラムの追加位置を気にせずに済むからです。 例えば GUI クライアントなどで DB に直接アクセスして中身を確認する時、大抵はタイムスタンプも合わせて見たいし、後からカラムがタイムスタンプの後ろに追加されていると、SELECT *した時にパット見でタイムスタンプを見つけにくいです。 MySQL ならafterでカラムの追加位置を指定できますが、指定せずに済むならその方が楽です。が、実際に業務で試した事はまだありません。

メソッド修飾子をステートレスにする?

Ruby にはprivateprotectedといったメソッド修飾子の書き方がいくつかありますが、メジャーなのは以下のように複数のメソッドにまとめて適用する方法だと思います:

def SomeController
  # public methods
  def index
    some_index_logic
  end

  def show
    some_show_logic
  end

  private

  # private methods
  def some_index_logic
    # ...
  end

  def some_show_logic
    # ...
  end
end

自分も普段はこの書き方ですが、実は個々のメソッドにインラインで記述するスタイルで統一しておく方がメリットは大きいような気がしています。

def index
some_index_logic
end

private def some_index_logic
# ...
end

これなら「公開されているか」に関係なく関連するメソッド同士を近くにおけるし、たとえ将来巨大なクラスに成り果ててしまってもスクロール中「どこからが private だっけ?」とならずに済むからです。 とはいえ、こちらも実際に業務で試した事はありません。

まとめ

まだ書き忘れてる事もありそうですが、とりあえずこの辺にしておきます。 主にコードのメンテナビリティについて意識する事を色々書きましたが、そもそもコードは正しく動く事が大前提というのも忘れてはいけないなと思います。 どんなに汚くて手をつけにくいコードでも正しく動いてるなら偉いし、逆にそれをないがしろにしたままコードや設計の良し悪しを議論するのは滑稽です。 ただこれらは別に相反するものではなく、むしろ複雑なユースケースであるほど、真っ当に設計する事が正しく動かし続けるためには有効であるはずだとも思います (Is High Quality Software Worth the Cost?)。

あとは書いていて、 Rails のレールに収まらない諸々のワークアラウンドをフレームワーク化すると Hanami に行き着く気がしました。 まだ触ったことはないのですが、機会があれば試してみたいです。 無論 Hanami が Rails の上位互換となるわけはないでしょうから、どんなトレードオフがあるのかを知りたいです。 あるいはどうせ自由に選べるならそもそも静的型付け言語にしたい気もしますが、なんだかんだ言ってもActiveRecordの便利さは日々実感します。 何を使うにしても、引き続き試行錯誤しながらコードを書いていきたいです。