Railsでやってしまいがちな保守性を下げてしまうコードとその解決策

こんにちは。COUNTERWORKSアドベントカレンダー13日目担当の疋田です。

先月からエンジニアとしてJOINしました。現在、業務ではshopcounterというサービスのRailsアプリケーション開発や日々の運用、データ集計や分析を元にしたプロダクトの改善などをメインで行っています。

スタートアップのエンジニアを経験していく中で、常に素早くPDCA回してユーザからのフィードバックをプロダクトに反映することが重要になってくるため、エンジニアとしてはコードの変更のしやすさとか捨てやすさ、読みやすさってかなり重要だなーと改めて強く思ってます。

今回は3年くらいRailsやってきた中でちょっとずつ溜まってきたメンテするときこういうコード辛かったなって部分を共有できたらなと思います。
ちなみに、これらはすべて今までの自分自身もやっていた時期があるコードであり、反省の意味も込めて書いてみます。

見通しの悪いコールバック

Viewに表示する情報が多くなったり、複雑になったりすると、コントローラのコールバックが増えがちになってしまうことがあります。

class UserController < ApplicationController
  before_action :load_user, only: [:show, :edit, :update, :destroy]
  before_action :load_users, only: [:index]
  before_action :load_roles, only: [:new, :edit]
  before_action :load_user_form, only: [:new, :create, :edit, :update]

  # ...

  def show
    @recommendation_items = user.recommendation_items
    @cart_items = user.cart_items
    # ...
  end
end

管理できる量ならとても便利でコードもdryに書けるのでどんどん使ってしまっていいと思うのですが、アクションごとに何が実行されているかわかりづらくなったり、インスタンス変数が多くなりすぎて、最終的にどんなインスタンス変数を保持しているのかわからないなどと感じたときはコールバックをやめるタイミングかもしれません。

そのときの対策の一つとして私はViewModelクラスを作って対応するが良い思ってます。また、普通にView用のデータ構造を作るのもコントローラやサービスでやるのではなくViewModelでやるのがいいのかなと。

class UserController < ApplicationController
  def show
    @view_model = UserShowViewModel.new()
  end
end

class UserShowViewModel
  include Virtus.model

  attribute :user, Integer

  def recommendation_items
    @recommendation_items ||= user.recommendation_items
  end

  def cart_items
    @cart_items ||= user.cart_items
  end

end

このように、Viewに渡すためのデータを管理するクラスを一つ作ってあげることで、そのクラスを見ればそのビューがどのようなデータを扱っているかも分かりますし、ビューやコントローラにロジックを書くことも防げます。

ビューを知りすぎたDecoratorクラス

コントローラからビューにモデルを渡すような場面で、そのままじゃ使えなくていろいろなViewでそのモデルを加工したり、結合したりして使っているような場合ではデコレータを使うことで、その見せ方の責任をデコレータに任せることができます。そうすることで、見せ方を変更したい場合でも、Decoratorクラスを変更するだけでよくなり保守性が上がります。またビューにロジックを書くようなことも抑えることができます。

ちなみにRailsで開発している場合 draper というGemを使うとDecoratorを簡単に実現できます。

class UserDecorator < Draper::Decorator
  delegate_all

  def fullname
    [object.sei, object.mei].join(' ')
  end
end

class UserController < ApplicationController
  def show
    @user = User.find(params[:id]).decorate
  end
end

<%= @user.fullname %>

適切に利用すると、保守性をあげることができるのですが、デコレータをビューに依存させてしまうことがあります。開発者はビューで特定の表示がしたいときにデコレータを活用しようとします。なので、気をつけていないと、そのビューに依存しすぎた作りにしてしまうことがあります。

例えば、以下のようなケースです。

class UserDecorator < Draper::Decorator
  delegate_all

  def profile
    "#{object.sei} #{object.mei} (#{object.age}) <br /> #{object.company_name}"
  end
end

class UserController < ApplicationController
  def show
    @user = User.find(params[:id]).decorate
  end
end

<%== @user.profile %>

このように特定のビューで使われることを意識しすぎたDecoratorメソッドを作ってしまうと、その場所でしか使えないメソッドとなってしまい汎用性が下がってしまいます。APIを作る際にも渡すデータは decorate してから渡したいかもしれませんし、他のページで使いたい場合もあるかもしれません。HTMLのタグなどが入ってしまうとユースケースが限定されてしまいます。

なので、デコレータに使うのは特定のビューに依存したものではなくモデルを表示用に整形するときだけが望ましいと思います。それにHTMLタグなどを付け加えるのはUI層のviewやhelperでやるべきものかなと。逆に言うとデコレータではビューがそのデータを使って特定の表示を作りやすいようなインタフェースとしてあげると使いやすいものになるかなと思ってます。

def fullname
  [object.sei, object.mei].join(' ')
end

def address
  [object.prefecture, object.city, object.town].join(' ')
end

ビジネスロジックの詰まったFormクラス -> Serviceクラスの活用

最近はFatModelをなんとかしようといろいろなプロジェクトでFormクラスが使われるようになってきたように感じます。検索や作成のフォームを作成する際に、form_withやform_forに渡すオブジェクトとして使っているケースをよく見かけます。
Formクラスも特定のフォームのバリデーションをモデルに依存せずにフォーム毎に変更できるようになりますし（バリデーションの分岐は辛い）、フォーム特有のデータもモデルを汚さずにFormクラスに定義できて、且つform_withやform_forを使えるので良いと思っています。

ただ、Formクラスに保存や検索処理のビジネスロジックを書いていってしまうと、かなり見通しの悪いコードになるし、Formクラスの役割が一気に増えてしまします。そうなるとFormクラスがドメイン層とアプリケーション層の両方を担うことになってしまうのかなと思います。

class SignupForm
  include ActiveModel::Model
  include Virtus.model

  attribute :avator, String
  attribute :username, String  
  attribute :email, String
  attribute :password, String
  attribute :password_confirmation, String

  validates :username, presence: true
  # ... その他多数のValidation

  def save
    if valid?
      # s3への画像アップロード
      # ユーザの登録
      # メール送信などの通知処理
      # ログ出力
    end
  end
end

これくらいの量ならまだ大丈夫ですが、これがもっと激しいコードになるとその一連のビジネスロジックの責任をFormクラスが担うのってやりすぎじゃない？他の場所でもこのビジネスロジック使う場合あるよね？そっちでもまた同じこと書くの？（上の例はちょっと悪いかもですね...）となります。FormクラスはそもそもUI層からのなんかしらの要求を受けるためのものとして機能してほしいので、バリデーション処理後はせめて他のビジネスロジックを扱うオブジェクトにメッセージを送るだけであって欲しいです。
そんな時Serviceクラスを使うとちょっとすっきりします。

class UserCreateService

  def initialize(params = {})
    @user_params = params
  end

  def call    
    # s3への画像アップロード
    # ユーザの登録
    # メール送信などの通知処理
    # ログ出力
  end
end

class SignupForm
  #...いろんな定義

  def save
    UserCreateService.call(self.attributes)
  end
end

以下記事のserviceモジュールをincludeしたものとしてください。
参考：https://techracho.bpsinc.jp/hachi8833/2017_10_16/46482

Formクラスはビジネスロジックを担ってくれる、Serviceクラスに対して命令するだけの役割とすればまだ、役割分割できてすっきりしそうです。

また、Formクラスに#saveを実装するのはちょっと反対です。それらをやらせるとFormクラスは役割を複数持つことになってしまう気がして、ちょっと単一責任じゃないかと。Formクラスはユーザから何かしらのリクエストを受け付けるためのインタフェースとしてのデータを持っていて、そのデータのチェック（バリデーション）をするまでの役割にするのがいいのではと思ってます。これに関して何か思うことがある方アドバイスもらえるとうれしいです。

散らばった更新処理

アプリケーションを開発していると更新処理はいろいろな場面で行うことになります。フォームから送信された内容を更新するときもあれば、データを論理削除にするために deleted_at を更新したり、時には下書きにするためにstatusにdraftを設定する場合もあるかもしれません。
その中でもドメインの中で決まった更新（削除やステータスの更新など）をする処理がいろいろな場所に散らばっていると、変更の際にとんでもなく辛いし、そういった部分はクリティカルであることが多いので、ちょっと怖いです。こういった処理をいろいろなコントローラでやってしまうプロジェクトはなかなか大変です。

それらを防ぐために基本的に更新処理はモデル(=ActiveRecord)に寄せるの良いと思っています。

class User < ApplicationRecord
  def soft_delete
    update(deleted_at: Time.zone.now)
  end 
  # 実際には論理削除などはモジュールになりそう  
end

class Article < ApplicationRecord
  enum status: { draft: 0, published: 1 }
  # enumを使うと published! や draft! といったメソッドが動的に作られる
end

アプリケーションによっては、決まったupdateが複雑なものもあるかもしれませんが、そういったものが仕様変更になったときでも一箇所だけ変えればいい状況を作っていくことが大切です。

デメテルの法則を違反したコード

デメテルの法則は別名を「最小知識の法則」といって、オブジェクトは自分が直接知っているオブジェクトにのみメッセージを送ろうということです。

Wikipediaには

あるオブジェクトAは別のオブジェクトBのサービスを要求してもよい（メソッドを呼び出してもよい）が、オブジェクトAがオブジェクトBを「経由して」さらに別のオブジェクトCのサービスを要求してはならない。

と書かれています。デメテルの法則に違反したコードは、必要以上の知識をオブジェクトに対して課すことになります。その結果、オブジェクトの持つべき知識は膨大になり、変更を加えた時に影響範囲がどこまで及ぶのかを管理できなくなります。経由しているオブジェクトに変更が加わった場合には、それを使っているオブジェクトにも変更を加える必要が出てしまい、変更のコストを高めてしまいます。

悪い例

@space.owner&.name

良い例

@space.owner_name

このようにすることで、@spaceにはowner_nameというパブリックインタフェースが生まれます。たとえ、ownerの返すオブジェクトがnameをnicknameという名前で持つようになったり、seiとmeiで持つようになった場合でもowner_nameのメソッドの振る舞いを変更するだけです。悪い例の場合にはそれを使っている全部の箇所で

@space.owner&.nickname
[@space.owner&.sei, @space.owner&.mei].join(' ')

などと書かなくてはならなくなり、変更コストが高まります。（実際にはdecoratorを使うかもしれませんが）
デメテルの法則について考えることは、本来必要であったパブリックインタフェースを見つける手がかりともなります。

なお、このようなデメテルの法則に違反したコードをなくすための方法として「委譲」という方法を使う場合があります。委譲では他のオブジェクトにメッセージを使えるように委譲します。
RailsではActiveSupport#delegate を使って委譲を簡単に実現することができます。

委譲を使うことで先程の処理はこのように実現することができます。

class Space < ApplicationRecord
  belongs_to :owner
  delegate :name, to: :owner, prefix: :owner, allow_nil: true
end

@space.owner_name

デメテルの法則への違反は、開発者がその先のオブジェクトがどんな振る舞いをするか、何を返してくるかということについて、知っているためにやってしまうことが多いです。ですが、そのオブジェクトの知っているべき範囲と責務を考えて、知るべきところだけを知り、知らなくていい範囲を他のオブジェクトに任せるということに向き合わなければ後々大変な依存を生み、改修困難なオブジェクトがたくさん生まれていくので気をつけたほうが良いと思います。

ダックタイピングを活かしていないコード

Rubyはダックタイピングが使える言語の１つです。オブジェクト自身ができることはオブジェクト自身が知っており、インタフェースを宣言的に実装していなかったとしても、その振る舞いができるオブジェクトはそのインタフェースを実装しているとみなすことができます。ダックタイピングは「もしオブジェクトがアヒルのように鳴き、アヒルのように歩くならばそれはアヒルである」という表現から由来しています。
つまり、Rubyにとってそのオブジェクトが何者であるかは問題ではありません。何ができるのかが重要なのです。これらを利用することで、設計に柔軟性と表現力が生まれます。でも、使い方を間違えれば、とてもリーダブルじゃないコードが生まれることも忘れてはいけません。全てはパブリックインタフェースへの信頼の上で成り立っています。つまりRubyはプログラマの知識を信頼しています。

Railsアプリケーションを作成する際にもダックタイプを見つけることで、拡張性のあるコードを書くことができます。case文を使って各オブジェクトにそれぞれの処理をしている箇所があれば、それはダックタイプを見つけることができるかもしれません。その振る舞いを抽象化してみるとダックタイプが見えて来る場合があります。

例えば以下のようなコードがあったとします。
以下のコードでは商品を出荷する前に一つ一つを確認しています。商品自体は別クラスのインスタンスとなっています。（ポリモーフィック関連とかイメージしてもらえると）

@user.items.each do |item|
  case item.class
    when Items::Food
      item.check_shape && item.check_color
    when Items::Clothes
      item.check_size && item.check_color
    when Items::Vehicle
      item.check_color && item.check_safety
  end      
end

しかし、考えてみると、商品を確認するという振る舞い自体はどのクラスも同じはずです。
そうした場合、確認するという共通の振る舞いを見出すことができます。それぞれのクラスに shipment_check メソッドを実装すると上のコードは以下のように書き換えることができます。

@user.items.each do |item|
  item.shipment_check
end

こうすれば、新たに商品のクラスが増えた場合でもこのコードに手をいれる必要はなく、新たにクラスを追加してshipment_checkメソッドを実装すればOKになり、保守性も上がります。

依存を含んだクラス

疎結合なコードを書くためには可能な限り依存を少なくする、隔離する努力が必要です。
よくやってしまいがちな、依存を生むコードとその解決策を紹介します。

特定のクラスに依存してしまっているケースです。

class Image

  def create
    url = Amazon::S3::Uploader.new(image, name).upload
    Image.create({url: url, name: name})
  end

  def update
    url = Amazon::S3::Uploader.new(image, name).upload
    Image.find(id).update({url: url, name: name})
  end

end

上のコードはAmazon::S3::Uploaderクラス、Imageクラスに依存しています。UploadeするのをS3から別のクラウドストレージにする場合には複数の箇所を変更しなければならなくなります。２つとかならまだいいですが、その数が大きくなればなるほど変更が大きくなります。結局のところcreateでもupdateでもuploaderはuploadメソッドを持っていることを期待しているので以下のように変更することで、依存範囲を限定することができます。

class Image
  def create
    url = uploader.upload
    image.create({url: url, name: name})
  end

  def update
    url = uploader.upload
    image.update({url: url, name: name})
  end

  def uploader
    @uploader ||= Amazon::S3::Uploader.new(image, name)
  end

  def image
    @image ||= Image.find(id)
  end
end

上のようにすることでuploaderもimageも依存させたいクラスを変更したい場合には、追加したメソッドを変更すればよくなりました。

次に外部メッセージへの依存を隔離する部分です。
先程の例で外部クラスへの依存は限定しましたが、まだメッセージは依然として依存したままです。uploadメソッド変えたくなるかもしれません。そうしたときには以下のようにすることで依存を限定することができます。

class Image
  def create
    url = upload
    image.create({url: url, name: name})
  end

  def update
    url = upload
    image.update({url: url, name: name})
  end

  def upload
    uploader.upload
  end

  def uploader
    @uploader ||= Amazon::S3::Uploader.new(image, name)
  end

  def image
    @image ||= Image.find(id)
  end
end

このように依存の範囲を限定してあげることで、変更に対して修正箇所を少なくすることができます。
（全てに対してこのようにすると見通しが悪くなることにもつながるので、そこら辺は臨機応変に）
もちろん、依存するのはある程度しかたないケースももちろんあるかとは思います。目的は、変更にかかるコストを抑えることなので、重要なのはその依存を如何に分離できるかだと思います。

次に、引数の順番に関する依存です。

class Image
  def self.resize(image_path, output_path, x, y)
  end
end

Image.resize('./sample.png', './sample-2.png', 300, 400)

こうした引数を必要とするメソッドがあった場合、2つ嫌な部分があります。まず1つはメッセージを送る側をパット見て引数のうち何が何を表していることがわからないことです。2つ目は引数の順番が変わった時にそれを使っている部分すべてを変えなくてはいけなくなることです。
そのような場合にはキーワード引数を使うことで回避することができます。

class Image
  def self.resize(image_path:, output_path:, x:, y:)
  end
end

Image.resize(image_path: './sample.png', output_path: './sample-2.png', x: 300, y: 400)

こうすることで２つの問題に対応できます。
また、引数にとるものがオプションの場合には以下のようにするのもいいかもしれません。

class Image
  def self.resize(options = {})
    image_path = options.fetch(:image_path, '')
    output_path = options.fetch(:output_path, '')
    x = options.fetch(:x, 300)
    y = options.fetch(:y, 400)
  end
end

FatView, FatController, FatModel

Railsで単一の責任の原則などのオブジェクト思考の原則を無視して作っていくと、アプリケーションが大きくなるにつれて、モデルやコントローラ、ビューが肥大化し、一つのクラスやメソッドが管理できないほどの役割を持ってしったり、いろいろなコントローラで同じ処理を書いてしまっていたり、いつのまにかViewにロジックいっぱい書いてしまっていたり、といったことが往々にして起こります。そうなると、一つ一つのクラスの役割が大きくなり、変更どころか処理追うのも大変になるようなコードが生まれます。

Railsというフレームワークの上だからといってオブジェクト指向の基本は一緒です。SOLID原則をしっかりと守って設計していかないと後々変更の度に必ず苦労することになるし、テスタビリティが低いコードになってしまいます。テストを書くのが大変になって、これはなんのためにテストしてるんだろうみたいな事になりかねません。

Rails開発で初心者の方に知っておいてほしいのは、ビジネスロジックを追加するためのモデルはActiveRecordだけではないということです。ActiveRecordはActiveRecordパターンとORMの特徴を持つgemであり、永続的なデータと、それに対するロジックを追加するものです。このクラスがすべてのビジネスロジックを請け負うものではありません。モデル層は責務に応じて色々なクラスがあっていいはずです。

Railsには責務分割した上でも使いやすいようにモジュールとしてActiveModelが存在し、ValidatorやCallbackも他のクラスとして切り出せるようにな仕組みが用意されています。

ここでは具体的なクラス分割やアーキテクチャの話まではしませんが、いろいろな記事があるので参考にすると良いと思います。個人的に導入してよかったと思っているのは、以下はよかったなーと思っています。

UI層

• view
• view_object

アプリケーション層

• controller
• decorator
• serializer
• form

ドメイン層

• model
• finder
• notifier
• service
• validator
• callback
• parameter

参考までに。

適切なクラス分割が出来ているコードは、変更するコストを下げることができます。取得系のロジックを変えたければfinderクラスを変えればいいですし、通知がメールからSlackになればnotifierクラスを変えればいいだけです。（技術的な関心ごとを直接書かない工夫は必要な場合はあります。それらはlibsにまとめるとか）気をつけて欲しいのは、意味が異なるのに同じような処理をしているからといって、同じクラスを使いまわしてしまうと、変更しづらいコードになってしまいます。

責務の分割はググってこうやるといいらしいで済ませるのは良くありません。本当にそのクラスがその知識を持つべきか、その振る舞いをするべきか、共通の振る舞いをする部分はないか、そこに抽象的なつながりはないかなどをチームとして考えることが大切だと思います。設計はアプリケーションの性質や規模、段階によって異なるため、一般的に使われてるから正しいとかそういったことはなく、どう分割するかを徹底的にチームで考えこみながら進めていくしかないと思います。抽象化したり、共通のロールを見つけたりと、設計はとても楽しいので、ぜひチームであーだこーだ言いながら話合うと良いと思います。

まとめ

本当はもうちょっと書きたいことがあった(Transactionとか例外とか、ログの残し方とか)んですが、もう疲れたのでここまでにします。。。

ということでまとめると、、初心者の方は

• 「オブジェクト指向設計実践ガイド」を読みましょう
• 「パーフェクトRuby on Rails」を読みましょう
• 「プロを目指す人のためのRuby入門」を読みましょう

ということです。
自分もまだまだ試行錯誤中&勉強中なので、いろいろ試していきたいです。