負債を抱えすぎたRailsアプリのリファクタリング

株式会社LITALICO のklrutsaです。
『LITALICO Advent Calendar 2016』3日目の記事です

はじめに

私が最初に携わったRailsアプリは以下のような、技術的負債を抱えていました

• Railsのレールから外れている
  • SQLべた書き、Validationを使っていない、Migrationを使っていない　等
• テストコードがゼロ
  • 人力テストで確認
• HTML、CSSが管理されていない
  • 一万行を超えるcss、さらにhtmlにstyleでべた書きという悪夢
• 複雑な権限、状態遷移
  • 無数のステータスがあるモデルが何種類かあり、さらにそれらが絡み合って複雑に動作する
  • 組合せ爆発のような事が発生している
  • すべての状態をテストできていないため、潜在的なバグがある

どこの会社でもあるあるだと思います。
こういう事が発生しないようにチームやマネジメントを改善するべきですが、
発生してしまった技術的負債からは逃れることができないので、ちゃんと向き合う必要があります。

で、どうやって対処するかですが、

• Railsのレールから外れている →　書き直せばいいだけで、そんな大変じゃない
• テストコードがゼロ　→　最初大変だけど、書けばいずれは解決する
• HTML、CSSが管理されていない　→　きれいに書き直すだけなので、ゴールは見えている
• 複雑な権限、状態遷移 →　一番つらい

というわけで、複雑な状態遷移をどうやって克服したかを書いていきます。

複雑な状態遷移

どんな言語であれプログラミングで一番書くコードは圧倒的にifなどの条件分岐が多いんじゃないのかなーと思ってます。
なので条件分岐を一番意識して書けば、必然と良いコードになると思います。

サービスを運用していると、色々な機能追加がされて、複雑な状態を持つということが発生します。
そうなると、必然的に条件分岐が増え、処理が肥大化して、保守がめんどくさくなる、ということになっちゃいます。

よく条件分岐の例だと

• ユーザー権限
  • 管理者、有料会員だけ見れる　等
• 時間
  • 最新の投稿ならNewと表示する　等
• 存在チェック
  • タグやいいねがあったらViewに表示する　等

こんな感じで想定される状態が増えれば増えるほど、分岐処理が多くなり、管理がしにくくなってしまいます。

状態が複雑になると

if (status == 'test' && updated_at > Time.zone.now ) || flag?
  # 処理
end

if user.present?
  unless user.follower.count >= 10
    # 処理
  end
end

<% unless !a || ( b && c ) %>
  <%# テキスト %>
<% end %>

というような読むだけで大変なifやunlessがたくさん出てきます。
絶対バグります。

というわけでこう言う、なんかよくわからない条件分岐が発生しちゃった時に、どう向き合ったかということを書いていきたいと思います。

例

例を出してどういう風に対処したかを書いていきます。

第一段階

class ArticlesController  < ApplicationController
  def update
    article = Article.find(params[:id])
    if current_user.admin? || ( current_user.writer? && article.status != 'published' )
      #updateの処理
    end
  end
end

※ current_userメソッドで、現在ログインしているユーザーを取得できる　とする。

Articleというモデルを更新する処理を考えてみます。
更新処理は

• 管理者（admin）は更新できる
• 公開済み（published）でないときはライター（writer）は更新できる

としましょう。

このコードには以下のような問題があります。

• 変更に弱い
• 読みにくい

もし、更新できる条件を増やして欲しいという依頼が来た時、if文を変更することになると思うのですが、ここから更に追加するとますます読みにくくなってしまいます。

また、こう言う似たような処理が他の箇所にたくさんあったりすると大変です。
条件分岐のルールが統一されていないので変更するコストが高くなってしまいます。

第二段階

最初はif分の中身をモデルに移して書き直しましょう。

class ArticlesController  < ApplicationController
  def update
    article = Article.find(params[:id])
    if article.editable?(current_user)
      #updateの処理
    end
  end
end

class Article < ApplicationRecord
  def editable?(current_user)
    current_user.admin? || ( current_user.writer? && self.status != 'published' )
  end
end

まず、if文が何を判定しているかを考えます。
ここではupdateの処理をするかどうかなので、編集できるかできないかを判定しています。
なのでeditable?というメソッドをArticleモデルに作りそこで判定を行うようにしました。

Articleモデルに書いた理由として、再利用性が高いからです。
このeditable？メソッドをdeleteなどの他のアクションで使いたい場合、容易に対応できます。

ちなみに、以下の方法は良くないと思います。

• コントローラーのprivateメソッドに書く
• （Viewの場合）Helperメソッドに書く
• インスタンス変数で@admin_flagみたいな感じでviewに渡す

ここではArticleの状態で判定を行うため、Articleモデルに聞くというのが素直で正しいと思います。

第三段階

class Article < ApplicationRecord
  def editable?(current_user)
    return true if current_user.admin?
    current_user.writer? && unpublished?
  end

  def published?
    self.status == 'published'
  end

  def unpublished?
    !published
  end
end

そして次は、ガード条件を使ったり、メソッドを小分けにします。
こうすることで、それぞれの条件分岐が何をしたいのかがわかりやすくなります。

第四段階

テストを書きます。（RSPec）

describe 'editable?のテスト' do
  shared_examples_for 'true' do
    it 'trueとなる' do
      expect(article.editable?(user)).to be_truthy
    end
  end

  shared_examples_for 'false' do
    it 'falseとなる' do
      expect(article.editable?(user)).to be_falsey
    end
  end

  context '管理者のとき' do
    let(:user) { create(:admin_user) }
    let(:article) { create(:article) }
    include_context 'true'
  end

  context 'ライターで公開済みの記事の時' do
    let(:user) { create(:writer_user) }
    let(:article) { create(:published_article) }
    include_context 'false'
  end

  #他の状態のときのテストを書いていく
end

上記の様にテストを書くことで、editable?メソッドの信頼性が格段に上がります。

メソッドのテストを図にして考えてみます。
メソッド内には条件分岐以外にも、レコード保存、更新など何かしらの処理を行っています。
メソッドのテストは入力と、条件分岐や処理を通って得られた出力を比較して、正しいかどうかを判定しています。

しかし、メソッドのテストですべての状態をカバーしようとすると、状態の組み合わせが多くなってしまい全ての状態をテストすることにコストがかかってしまいます。

なので、以下のように条件分岐ごとにテストを追加した方が良いです

条件分岐がしっかりとテストされていれば、どのような状態に対しても、テストによって保証された順序で実行されることになります
メソッドのテストは細かくやらずに、メソッド内の条件分岐のテストを細かくやっておけば、ほぼ十分にテストされているといえます。

終わりに

大前提として、複雑な物を作らないということこそ正義ですが、
色々あって複雑な状態を抱えるシステムを保守しなければならないことになると思います。

そういうときは、まず条件分岐を整理して共通化してテストを書いておけば、保守や運用が行いやすくなると思います。

明日は、Takuan_Oishiiさんの超初心者向けWebアプリ公開です。お楽しみに