大規模システムのリファクタリング

この記事は食べログアドベントカレンダー2022 の2日目の記事です🎅🎄

この記事を書くきっかけ

食べログのような大規模システムで既存機能改修を行う際、構造把握に時間がかかることがあります。
なので、今後の開発がスムーズに進むようにリファクタリングを行いたいと考え計画を立てました。
そして計画を立てた時の内容が役に立つかと思いまとめてみました。

リファクタリングしたい

既存機能の改修時に以下の課題を感じて、リファクタリングが必要と考えていました。

散らばっているビジネスロジックを集約して対応箇所の調査を簡単にしたい
修正/テスト工数を下げたい
複雑すぎて対応工数がかさむ（主にテスト）
既存機能の修正で見落とす

リファクタリングが進まない

とはいえ進めたい案件が並んでいたり、どこからやるの？というのもあって、思ったようにはリファクタリングは進みません。

案件を優先したい、忙しい
対応コストが大きい
どこから手をつけていいのかわからない
そもそも仕様が複雑すぎて、限界があるのでは？

リファクタリングを進める

開発部内だけで問題を個別にリファクタリングしても埒があかないので、以下のようにリファクタリングの全体の筋道を立てていきました。

コードの調査や案件対応の中で発見した課題箇所を整理
リファクタリング計画を立てる
企画にリファクタリングについて効果を説明し工数をもらう（平たく言えば案件化する）
リファクタリング実施

1. 既存調査や案件対応の中で発見した課題箇所を整理

大まかな構成図を作り問題のありそうなところをざっくりと出す
クラス図やER図を作成し、問題のある箇所を洗い出す
案件対応中に見つけた課題を積んでいく
- 既存の判定ロジックを変更するような対応
- 特に大型案件対応では課題が顕在化しやすいため見つけやすい

2. リファクタリング計画を立てる

■調査内容からリファクタリングの内容を洗い出す

現状の課題（例）
- 利用されていないコードが存在する
- 参照、判定系のロジックが冗長で調査に時間がかかる
- メタプログラミングされている箇所が調査に引っ掛からず、バグを誘発する
- 更新系のロジックが巨大で対応工数がかかる
- 色々な箇所にロジックが散らばっていてわかりづらく、修正も大変
- 仕様が複雑すぎて、対応が漏れる

■課題の対応方針を考える

上記課題（例）への対応
- 利用されていないコードにログを仕込んでアクセスされていなかったら削除
- 冗長な参照、判定をリファクタリング
- メタプロミング部分をハードコーディングにリファクタリング
- 更新系のロジックをユースケースごとに分割
- 散らばっているロジックを集約して別システムに切り出す
- 不要な仕様を削除するよう企画と仕様面含めたリファクタリングを進める

■優先度を決め、スケジューリング

良い影響が大きいもの、対応が少なくて良いものから実施
例の中だったら「参照、判定系のロジックが冗長」な箇所から対応など
- 既存の判定が散らばっており集約すれば大きな変更時に調査や対応が簡単になる
- ある程度ロジックが簡便なため、RSpecが作成しやすく品質担保しやすい

3. 企画にリファクタリングの効果を説明し案件化する

企画にリファクタリングについて説明し、案件化や仕様削減についてゴーサインをもらいます。
普段から企画とはブレストや振り返り会などで密にコミュニケーションをとっているので
こういった案件外のことも相談しやすい雰囲気があります。
ブレストでエンジニアが施策のアイデアを提案することもあります。
その中で今回のリファクタリングの話も提案しています。

作成した資料を元に、現状の課題とリファクタリングの内容について説明する
リファクタリングにより構造が整理され、案件のスピードが向上するということを説明
（隙間作業ではなく）案件として進めるということにゴーサインをもらう
またロジックが複雑化するような既存仕様を削れないか調整も行う

4. リファクタリング実施

計画どおりフェーズを案件化して進めていく
RSpec実装→リファクタリングのサイクルを回していく
1. リファクタリング部分のみにRSpec実装箇所を絞る
2. 冗長化することに目をつぶって呼び出し元の全てのテストパターンをカバーする
3. リファクタリング実施
4. リファクタリング完了後に通常のRSpecに戻す

例）リファクタリング前のロジックとRSpec

# リファクタリングしたいロジック
def status_publish?
  true unless status == 1 && code == 'A'
  false
end

# 利用している箇所（ここの挙動を担保したい）
def execute
  return 'AA' if status_publish?
  return 'BB'
end

describe "#status_publish?" do
  #全パターンのテスト
end

describe "#execute" do 
  context "status:1 code:A" do
    it "BBが返却される" do
      FactoryBot.create(:example, status:1, code:'A')
      expect(example.execute).to eq('BB')
    end
  end
  context "status:1 code:B" do
    it "AAが返却される" do
      FactoryBot.create(:example, status:1, code:'B')
      expect(example.execute).to eq('AA')
    end
  end
  context "status:2 code:A" do
    it "AAが返却される" do
      FactoryBot.create(:example, status:2, code:'A')
      expect(example.execute).to eq('AA')
    end
  end
  context "status:2 code:B" do
    it "AAが返却される" do
      FactoryBot.create(:example, status:2, code:'B')
      expect(example.execute).to eq('AA')
    end
  end
end

例）リファクタリング後のロジックとRSpec

# リファクタリングしたロジック
def status_publish?
   true if status == 1 && code == 'B' 
   true if status == 2 && code == 'A'
   true if status == 2 && code == 'B'
   false
end

# 利用している箇所
def execute
  return 'AA' if status_publish?
  return 'BB'
end

describe "#status_publish?" do
  #全パターンのテスト
end

# 担保されるので２パターンのみ実施でOK
describe "#execute" do 
  context "status:1 code:A" do
    it "BBが返却される" do
      FactoryBot.create(:example, status:1, code:'A')
      expect(example.execute).to eq('BB')
    end
  end
  context "status:1 code:A以外" do
    it "AAが返却される" do
      FactoryBot.create(:example, status:1, code:'B')
      expect(example.execute).to eq('AA')
    end
  end
end