見出し画像

PKSHA FAQ のリファクタリング&リアーキテクチャの取り組み

こんにちは、PKSHA Communication で PKSHA FAQ 開発チームのテックリードをしている清水です。PKSHA FAQ は Ruby on Rails で作成されています。本稿では PKSHA FAQ が現在取り組んでいるリファクタリング&リアーキテクチャの一部をご紹介させていただきます。

PKSHA FAQ の詳しい説明はこちらの記事をぜひご覧ください。

清水 勇太(CS 事業本部 FAQ 事業部 ソフトウエアエンジニア)
前職にてソフトウエアエンジニア〜フルスタックエンジニアとして経験を積んだ後、PKSHA Communication  へ入社。入社後は複数の新規サービスをメインで立ち上げ、現在は PKSHA FAQ の開発チームのテックリードを手掛けている。


課題

アーキテクチャの寿命に関してはさまざまな見解がありますが、個人的には概ね 5 年程度が一つの目安であると考えています。実際に PKSHA FAQ のソースコードの Git ログを確認したところ、初回のコミットは 2012 年 7 月に遡ることがわかりました。PKSHA FAQ はすでに 10 年以上にわたり運用されており、その間、ビジネスの成長と加速が最優先事項となっていました。

その結果、アーキテクチャやコードベースの見直しが後回しにされがちだったのが現実です。しかし、現在はその問題を解消すべく、少しずつではありますが、改善に取り組んでいます。
具体的な問題点として、今回は以下を取り上げてみます。

  • Fat Controller

  • Fat Model

  • 密結合

それぞれを掘り下げると、

Fat Controller, Fat Model

それぞれ、Controller や Model のソースファイルが非常に大きくなってしまっている状態を指します。Rails は伝統的な MVC モデルに基づくフレームワークであるため、基本的にはバックエンドの処理を Controller または Model に記述することになります。このため、適切な機能の分離が行われない場合、ミドルウェアを扱う Controller や主要なドメインを表す Model は自然と肥大化してしまいます。継承や Concerns を活用することで、これらの肥大化を防ぎ、よりクリーンなコードベースを維持することが重要です。

密結合

明確なルールを示さずに処理を実装していくと、Controller や Model、さらには View にも実装できてしまう処理が増えてしまいます。「どこに書けばいいんだろう?」や「サブルーチンの粒度はどのくらいが適切か?」といった疑問を気にせずにコードを配置してしまい、処理の責務や結合度が高まることになります。
また、前途の通り、継承や Concerns を使って機能を分離することは重要ですが、これらの手法だけではソースファイルの大きさを改善するに過ぎません。これらはあくまで外部のコードを自身に取り込む手段であり、生成されるインスタンスが密結合になってしまうリスクがあります。疎結合を実現するためには、さらなる設計の工夫が必要です。

これらを回避する為に、処理を分割できる場所でありつつ、Controller でも Model でもない別のレイヤーを用意していきます。

対応

ここからはサンプルコードを交えながら、ゴールイメージと照らし合わせていきます。(本当はサンプルではなく実運用のコードを掲載し、空気感も含めてお伝えしたかったのですが、諸事情あり断念しました。実運用中の fat なコードだと、Controller 1 ファイルで 6000 行オーバー!、Model 1 ファイルで 5000 行オーバー!のコードがちらほらあります)

Controller

以下のような Controller があるとします。
現時点ではあまり fat ではありませんが、処理を移行してみます。

class UsersController < ApplicationController
    before_action :get_login_user
    def get_login_user
        @login_user = User.find_by(id: request.session[:login_user_id])
    end

    def index
        @user = User.find(params[:id])
    end
    def update
        raise "no login" unless @login_user

        raise "name error" if params[:user][:name].blank?
        raise "name error" unless params[:user][:name].is_a?(String)

        @user = User.find(params[:id])
        @user.attributes = params[:user]
        @user.update(@login_user)

        render action: :index, locals: { user: @user }
    end
end

Controller 内の 2 つの action、 UsersController#index と  UsersController#update の処理自体を定型のフォーマットで別のクラスへ書き直します。

このとき、処理に必要なパラメータとして、Controller の世界にしか存在しない request や params といったものを利用しないように注意します。また、同様に render 等も利用しません。

class UsersIndexInteractor
	  Props = Struct.new()

	  Context = Struct.new(
	      :id
	  )

	  Result = Struct.new(
	      :user
	  )
	
	  def initialize(props)
	  end
	
	  def call(context)
	      user = User.find(context.id)
	
	      return Result.new(
	          user: user
	      )
	  end
end
class UsersUpdateInteractor
	  Props = Struct.new()

	  Context = Struct.new(
	      :login_user_id,
	      :id,
	      :attributes
	  )

	  Result = Struct.new(
	      :user,
	      :error
	  )
		
	  def initialize(props)
	  end
	
	  def call(context)
		    login_user = User.find_by(id: context.login_user_id)
		
		    error = "no login" unless login_user
		
		    error = "parameter error" if context.attributes.blank?
		    error = "name error" if context.attributes[:name].blank?
		    error = "name error" unless context.attributes[:name].is_a?(String)
		
		    return Result.new(
			      user: nil,
			      error: error
		    ) if error
		
		    user = User.find(context.id)
		    user.attributes = context.attributes
		    user.update(login_user)
		
		    return Result.new(
			      user: user,
			      error: nil
		    )
	  end
end

Controller からの呼び出しは定型で以下のようになります。Controller の世界にしか存在しない情報を与えて実行し結果を render 等へ渡します。

class UsersController < ApplicationController
    def index
        props = UsersIndexInteractor::Props.new
        interactor = UsersIndexInteractor.new(props)
        context = UsersIndexInteractor::Context.new(
            id: params[:id]
        )

        result = interactor.call(context)

        @user = result.user
    end
    def update
        props = UsersUpdateInteractor::Props.new
        interactor = UsersUpdateInteractor.new(props)
        context = UsersUpdateInteractor::Context.new(
          login_user_id: request.session[:login_user_id],
          id: params[:id],
          attributes: params[:user]
        )

        result = interactor.call(context)
        if result.error
            raise result.error
            return
        end

		    render action: :index, locals: { user: result.user }
    end
end

これで Controller から処理の内容に対する関心を抜き出すことができました。Router としての責務のみを行う形に近づいたかと思います。また、before_action を経由した不要なインスタンス変数を保持しなくなったことで Helper での事故防止などにも繋がります。

処理を抜き出した各 Interactor は、request や params といった外的な状態を意識しないで済むので、PORO のような扱いで rails console からの単体実行や依存のない単体テストをすることができます(ActiveRecord への依存があるのでアプリケーション外からの実行はまだ難しいですが・・・)

Model

続いて、 UsersUpdateInteractor で実行している user.update を見てみます。こちらも現時点では全く fat ではありませんが、処理を移行してみます。

class User < ActiveRecord
	  def update(login_user)
		    self.save!
		
		    if self.id != login_user.id
			      Mailer.send_mail(to: self, type: :edit_user, context: self)
		    end
	  end
end

定型のフォーマットで別のクラスへ書き直します。

class UsersUpdateService
		def update!(user, login_user)
		    user.save!
		
		    if user.id != login_user.id
			      Mailer.send_mail(to: user, type: :edit_user, context: user)
		    end
	  end
end

UsersUpdateInteractor は Service の Interface を受け取る想定で初期化し、実行するようにします。

  class UsersUpdateInteractor
    :
    :
      Props = Struct.new(
+         :update_service
      )
    :
    :
      def initialize(props)
+         @update_service = props.update_service
      end

    def call(context)
    :
    :
+       @update_service.update!(user, context.login_user)
-       user.update(context.login_user)
    :
    :

Controller からは本実装しているものをインスタンス化して渡します。

  class UsersController < ApplicationController
    :
    :
      def update
-         props = UsersUpdateInteractor::Props.new
+         props = UsersUpdateInteractor::Props.new(
+           update_service: UsersUpdateServiceAr.new(UsersUpdateService::Props.new)
+         )
    :
    :

最後に Model から、移行した処理を削除すれば完了です。

  class User < ActiveRecord
-     def update(login_user)
-         self.save!
-
-         if self.id != login_user.id
-             Mailer.send_mail(to: self, type: :edit_user, context: self)
-         end
-     end
    :
    :
  end

これで Model から User を更新するビジネスロジックを分離できました。

まとめ

ここまでの結果を見て、お気づきの方もいるかもしれませんが、今回の実装はクリーンアーキテクチャを大きく意識したものとなっています。

引用:The Clean Code Blog

Controller とドメインルールである Interactor や Entity を分離することで、一時的なコード量を対価にそれぞれのメンテナンスコストを下げています。
ActiveRecord に関しては、Data Access の Interface でありながら実装となることの扱いづらさを吸収するべく、自動的に生成されるメソッド以外を実装しないことを目指し、おおよそ Gateway に特化させようとしている形となります。以後は他のフレームワーク同様、呼び出し順序を意識していけば依存関係の崩壊や密結合は回避していけるはずです。

また、本稿では紹介しませんが、実際の PKSHA FAQ では他にも以下のような取り組みと組み合わせることで、効果の最大化を目指しています。

Sorbet

Ruby の世界に型を意識させるものです。
単純にメソッドの引数や戻り値がどのような型なのかを記載することで、実行時に自動的に assert させたりできる他、Struct や Const が Ruby の標準のそれより強力になっていたり、Interface 概念を取り入れることもできます。
依存関係が見通せる設計状況なアプリケーションであれば不要かもしれませんが、そうでない場合は局所的にでも導入することで想定外の型の不一致を洗い出すことができ、徐々にメンテナンスコストが下がります。

Packwerk

モジュラモノリスを前提としたモジュール間の依存関係を調査する解析ツールです。
実際の運用環境においては今回のサンプルのようにきれいに一つのドメインに特化した処理であることは稀で、ドメイン間の依存関係を意識して処理を分割していくことになります。
その際、例えばドメイン単位でモジュールを切って解析することで、モジュール外に公開する必要のあるビジネスロジックと、モジュール内に閉じたビジネスロジックを明確化することができ、公開範囲を狭めるということは影響範囲の最小化、テスト範囲の最小化につながります。

おわりに

今回は、PKSHA FAQ が現在取り組んでいるリファクタリング&リアーキテクチャの一部をご紹介させていただきました。この他にも、「リーダブルで静的解析ができるコード化」であったり、よりクリーンアーキテクチャに近づける為の「プレゼンテーション層の構築とミドルウェアとの棲み分け」等といった課題にも取り組んでいるので、また機会がありましたら紹介できればと思います。

実際、この取り組みはまだ道半ばであり、先は長いですが、大切なのは継続して進めていくことです。また、将来的に別のリアーキテクチャが必要になることを考慮し、その時に実行しやすくするための布石として、コードベースの型化やテストカバレッジ率の向上を行うことが重要だと考えています。

また、本番運用環境化でこのような取り組みを行うのは非常にチャレンジングですが、それだけにソフトウエアエンジニアとして成長できる貴重な機会でもあります。一緒に携われる仲間も募集中ですので、興味をお持ちの方、一緒に挑戦したいという方は、ぜひお声がけください。

―INFORMATION―
▼ 中途採用:ソフトウエアエンジニアの募集要項はこちら

▼ 中途採用:全職種の募集要項はこちら

▼ PKSHA 採用サイト

▼ Wantedly はこちら:カジュアル面談も受付中です!