Railsアプリケーションの技術的負債3つと、そのリファクタリングについて

はじめに

こんにちは！ZealsでRailsエンジニアをしている鈴木です。
みなさんは定期的に技術的負債の返済をしていますか？
今回は我々が日頃リファクタリングによって負債を返済する活動の中で得た気づきについてご紹介したいと思います。

はじめに
技術的負債とは
弊社のRailsプロジェクトにおける技術的負債
最後に

技術的負債とは

まず始めに技術的負債についておさらいをしておきましょう。
技術的負債とは、プロダクトを継続的に開発していく中で発生した、コピー&ペーストによるコードの重複、メンテナンスされない古いコードやライブラリの利用、テストコードを書かなかったことによるカバレッジの低下などによって、その後の機能追加開発やバグ修正などにかかる工数が通常よりも大きくなってしまう状態のことを言います。
コードの重複があると、修正対象がその中にあった場合には何箇所も修正しなければいけません。もしかしたら修正漏れが発生するかもしれません。メンテナンスされないライブラリを利用していると予期せぬバグを踏んでしまうかもしれません。テストコードがないことで想定外の動作に気づけないことになるかもしれません。
このような状況が積み重なっていけば、「その後の機能追加開発やバグ修正などにかかる工数が通常よりも大きくなってしまう」ことが容易にうなずけますね。最悪の場合、スクラッチから作り直したほうが早いなんてことにもなりかねません。
その様子を下図が如実にあらわしています。
f:id:ssabcire:20200731192135p:plain

図1は技術的負債を定期的に返済した場合と、そうでない場合とで要する開発工数の違いを表しています。定期的に返済した場合（赤線）では開発期間が25ヶ月経過しても1〜2の間に収まっているのに対し、返済をしなかった場合（青線）では返済し続けた場合の10倍以上に上っていることがわかります。
ちょっとした修正をするだけでこれだけの工数がかかるようになってしまうとエンジニアのモチベーションを維持するのも大変でしょうね。

弊社のRailsプロジェクトにおける技術的負債

弊社では日頃から積極的にリファクタリングを行い、技術的負債の返済に取り組んでおります。つまり図1の赤線のような状態を維持するように努力しております。しかしある一定水準から開発工数が低下しませんでした。とあるご縁から外部の方の技術協力を得ることができ、その方に見てもらったところ、まだ負債が存在していると判明しました。指摘されたのは以下の３点です。

Model内に複雑なクエリを書いていた
Serviceクラスの書き方に統一性がなかった
システム利用者とエンジニア間で異なる単語を使用していることで、同じ対象を示す単語が複数存在していた

これらを以下で詳しく見ていくことにしましょう。

1. Model内の複雑なクエリ

TODOリストを管理する 4 つの Model (TodoGroup, Todo, Task, Comment) があったとします。それぞれ TodoGroup → Todo → Task → Comment という方向で1対多の関係があると仮定します。そして Comment モデルには誰がコメントをしたのかを表す user_id を要素として持っていることにします。
この時、我々のアプリケーションでは、誰がコメントをしたのかを検索するためのSQLの定義を以下のように記述しておりました。

class TodoGroup
  has_many :todos

  scope :by_comment, lambda { |user_id|
    joins(todos: {
      tasks: {
        :comments
      }
    })
    .where(todos: {
      tasks: {
        comments: { user_id: user_id }
      }
    })
  }
end

ActiveRecordはそのテーブルに関する内容を実装するものです。したがって、クラス自身が知らなくてもいいことについては実装する必要はありません。上の例では TodoGroup が本来知らなくてもいい Task の中の Comment まで知ることとなっています。
そこで TodoGroupQuery というクラスを作成して、上記 scope の代わりとなるメソッドを定義することにします。

class TodoGroupQuery
  attr_reader :relation

  def initialize(relation = TodoGroup.all)
    @relation = relation
  end

  def search_comment(user_id)
    @relation.joins(todos: {
                tasks: {
                  :comments
                }
              })
             .where(todos: {
                tasks: {
                  comments: { user_id: user_id }
                }
              })
  end
end

こうすることによって、TodoGroup モデルは Comment を知る必要がなくなり、複雑なSQLをメソッド内部に隠蔽することができます。すなわち本来の責務である「テーブルを抽象化する」こととは関係のない処理を定義しなくて済みます。

2. Serviceクラスの統一性

Serviceクラスが乱立して書き方に統一性がなかったことから、初めてコードを見る人の学習コストが高くなっていました。そこで以下の方針で整理することにしました。すなわち

ドメイン駆動設計で定義されているドメインサービスの概念を導入し、ステートレスに Service クラスを記述する

例えば以下の例を見てみましょう。このような書き方をしてしまうと、Serviceクラスの状態を変更できてしまいますね。

  def initialize(todo_id)
    @conn = Sender.new(url: url do |faraday|
      faraday.request :url_encoded
      faraday.response :logger
      faraday.adapter Faraday.default_adapter
    end
    @todo = Todo.find(todo_id)
    send
  end

  def send
    res = @conn.post do |req|
      req.url(@service_url)
      req.headers['Content-Type'] = 'application/json'
      req.body = @todo.to_json
    end
    return true if res.status == 200
  rescue Faraday::Error::ClientError, Faraday::Error::ConnectionFailed => e
    result = JSON.parse(res.body).symbolize_keys
    SlackNotifier.new.send(err_msg(e, result))
    raise Error, err_msg(e, result)
  end

これを以下のように修正します。こうすることによって、Serviceクラスは状態を持たなくなり、その結果、同じ引数を受け取った場合に同様の結果を返すこと（冪等性）が保証されます。

def self.execute(todo_id)
    conn = Sender.new(url: url do |faraday|
      faraday.request  :url_encoded
      faraday.response :logger
      faraday.adapter  Faraday.default_adapter
    end
    todo = Todo.find(todo_id)
    send(conn, todo)
  end

  def self.send(conn, todo)
    res = conn.post do |req|
      req.url(@service_url)
      req.headers['Content-Type'] = 'application/json'
      req.body = todo.to_json
    end
    return true if res.status == 200
  rescue Faraday::Error::ClientError, Faraday::Error::ConnectionFailed => e
    result = JSON.parse(res.body).symbolize_keys
    SlackNotifier.new.send(err_msg(e, result))
    raise Error, err_msg(e, result)
  end

3. システム利用者が使用する用語での実装

システム利用者（事業部側の人）とエンジニアとの間で同じ対象を意味する異なる用語を使っていたことによって、両者間で共通認識を取るのが難しいことがありました。そこでアプリケーションを以下のような方針で修正することにしました。

両者間共通の用語を定義する層 (ApplicationServiceLayer) を作成
用語の乖離が発生した箇所の利用者側の用語への変更

たとえば実例を挙げると以下のようになります。Todoリスト内のタスクを更新するために必要な処理をすべて execute 内に 簡潔に 記述します。そして個々の詳細な処理についてはメソッド化するというものです。こうすることにより、execute メソッドを見るだけで、UpdateTask クラスが何をするためのものなのかが明確になり見通しが良くなります。

module TodoContentContext
  class UpdateTask
    class << self
        def execute(task_ids, todo_id)
            if invalid_task_ids?(todo_id, task_ids)
              return { error_message: 'Todoと関連付けられていません' }
            end

            update_params = build_update_params(task_ids)
            tasks = update_tasks(task_ids, update_params)
            send(tasks)
            { updated_tasks: tasks }
        end

        def update_tasks(task_ids, update_params)
          #tasksをアップデートする処理が記述してある
        end

        def send_email(tasks)
          #アップデートしたtasksを通知する処理が記述してある
        end
    end
  end
end