この記事は CrowdWorks Advent Calendar 2017 の9日目の記事です。

表題の件ですが、僕もです( ^ω^ )ﾆｺﾘ

今いるチームは開発を新規でどんどんやるチームではなくどちらかというと今あるサービスを安全に当たり前に使い続けるために変更を加えつつついでに整理する事が多いです。

突然ですが、こんな経験はありませんか？
あなたは既存のメソッドに機能を追加しようとしています。しかし、そのコードたちはもともと煩雑に作られている上に建て増し建て増しをされたことがわかる程度の雑さで重ねられ、もはやだれも相手できないモンスターに。。。

ひのきのぼうを渡したら討伐してきてくれる勇者は現実世界にはいないので、なんとか自分たちでやるしかないですね。

新規でつくるならこうするのに。。。
ここに追加機能を足したいからまずはここをこうしてこうしてこうして...わ～( ;ﾟ皿ﾟ)ﾉｼΣﾊﾞﾝﾊﾞﾝ!!

よくありますね！

そういうコードとエンカウントしたときに実践していることを紹介します。

まずテストは必ず書く！

リファクタリングしたつもりがバグってて本末転倒になったなんてことにならないように必ずやりましょう！

テストのないコードは信頼性も薄いですが、何より何をしているのコードなのか理解するのに無駄に時間を使います。
テストは完全に仕様をまとめたドキュメントにはなりえないですが、それでも全体感をつかむ上では大変有要です。
機能を追加する以前にリファクタするにも最初にやりましょう。

そして一番時間のかかる工程になるのでだいたいみんなここで諦めて同じように追加だけします。

サービスの初期段階では時間の関係上どうしてもできないときがあるかもしれませんが、いつまでも放置はしないようにしましょう。

以下テストを書く3ステップを紹介します。

ステップ1: この処理は何をしてるの？を知る

作った背景や気をつける点などをしっかり理解しないと、思わぬ副作用があるコードを書いてしまうためそうならないように必ずはじめにやりましょう！

やり方としては作った人に仕様を説明してもらうのが正直一番手っ取り早いです。

何年も続くサービスだと作成者がいない場合が多いですが、もしまだいるなら隠れている前提条件、はまりどころ、そういうポイントがまとめて聞けます。1からコードを読んで推察するより忘れかけの記憶のほうがいくらかマシです。
もしかしたら検索しても見つけられないメモ程度にまとめられたドキュメントが発掘できるかも。(大体古くなっているのであくまで全体感の把握程度に)

もし誰も仕様を把握している人がいなければ複数人で読みながら整理しましょう。

ステップ2: とにかく書く

仕様がわかったのでここからは単純な作業です。書きましょう。
どんなにテスト項目が多かろうが入り組んでいようが書き上げて下さい。

書かないことにはどこが悪いかもわかりにくいです。書いて初めて見えてくる矛盾もあるので書き上げるまでは一旦リファクタのことは忘れてやりましょう！

ステップ3: リファクタできそうな箇所を特定する

日頃仕事ではRspecでテストを書いてますが、個人的なポイントとしてcontextのネストの深さで見ています。

describe 'Aの処理' do
  context 'Bのとき' do
    it 'Cであること'
  end

  context 'Bじゃないとき' do
    context 'Dのとき' do
      context 'Eのとき' do
        it 'Fであるとこと'
      end

      context 'Eじゃないとき' do
        it 'Gであるとこと'
        it 'Hであるとこと'
      end
    end
  end
end

サンプルが適当でわかりにくいですが、Eじゃないとき以降のところは切り出しせるかな？のような見かたをします。

ネストが深くなりすぎているということはそこに行き着くまでの観点が多く、また、まとまった単位で切り出せる可能性があるということです。
特にflag, type等の属性で分岐が入る箇所はそれ以降の部分を切り出せる可能性があります。

以下のコマンドでアウトラインだけ確認できるのでテストが長すぎて見にくいときは使ってみて下さい。

$ bundle exec rspec -f d --dry-run --order defined file_name

Aの処理
  Bのとき
    Cであること
Bじゃないとき
  Dのとき
    Eのとき
      Fであるとこと
    Eじゃないとき
      Gであるとこと

小さく関心事に切り出す

あとは楽しいコーディングの時間です。
作業としては正直ここまでで70%~80%は終わったと思っていいでしょう。

やり方として先程も少し書きましたが、切り出せそうなところを探してみるのがいいでしょう。
簡単なのは単純にメソッドに切り出す、またはいっそ関心事でクラスに切り出してしまいましょう。

単純にメソッドに切り出す場合

# 単純にメソッドに切り出す場合

def Eじゃないときの処理
  # Gであることにする処理
  # Hであることにする処理
end

いっそ関心事でクラスに切り出して見る場合

Class Eじゃないクラス
  def initialize
    rails ArgumentError if Eだったら
  end

  def EじゃないときのAの処理
    # Gであることにする処理
    # Hであることにする処理
  end
end

メソッドに切り出すのは日頃よくやるかと思いますが、
クラスに分けて初期化時に前提条件をもたせることで

Eのときは全く関係ないから考える必要がなくなる。
今後同じEの状態のみに特化した処理を追加したい時に迷うことなくこのクラスに追加できる。

などのメリットがあります。

さらにここまで読んでもらえればわかるかと思いますが、メソッド名やクラス名に Eじゃない ようなふわっとした名前はつけれないので新たに共通認識を持てる言葉で表現できるようになります。

これは開発をやりやすさを向上させるだけでなくPO等のエンジニア以外の人とのコミュニケーションの上でもユビキタス言語として機能します。(開発するときも英語名でいちいち悩むことが今後なくなるので効率も上がります。)

逆にデメリットとしては

やりすぎると同じようでちょっと違う、全く汎用性のないクラスが乱立する。

適材適所で頻出頻度の高い場合はやってみて下さい。。。

おまけ

最後に諦めるでも最低これだけはやっておきたいことを紹介します。

TODO, FIXMEなどコメントだけ書いておく

メインの業務があり、触る機会があってもできれば無難に通過したいことがあります。
ひどいコードに出会ったときにちょっとリファクタできるか考えてみる。時間的、モチベーション的に今回は諦めようってなったときにはこういう観点で考えて諦めた等書いてあるとないより足しになります。

※ 余談ではありますが、CrowdWorksのコードの中には # HELPME: xxxxxxx と書かれているところもあります。だれがだれに向かって叫んでるのかは不明ですが、切実ですね。こうなる前に少しずつでも解消できるようにしましょう。

TODOコメント等が増えすぎるとTODOがただのコメントと化してしまう問題もありますが、ないよりましです。

すぐやる > TODOを書いてチケット化等やる予定を立てる >>> TODOだけとりあえず書く >>>越えられない壁>>> 見て見ぬふりをする

まとめ

一度に100%満足のいくリファクタは不可能。
少しずつルール化し、まずはエンジニア内で共通認識を持ち、ユビキタス言語としてまとめていくのが理想。
それでも今サービスが動いてユーザーに価値を届けている訳だから荒れ地に獣道を作った人はすごい！と開き直ることが精神安定上大事。
なんでもかんでもやりすぎると別の形で後で負債になるときもあるのでそこは全体で共有しつつ進める。

リファクタ、マイクロサービス化、作り直しが必要になるぐらいサービスが成長したってことはそれだけですごいよね！
サービスを前に進める勢いは大事。でもそろそろ既存の負債で勢い出せなくなってきたぞってなったときじゃあどうするを考えてみるのもありかもですね:D

既存サービスをリファクタしながら進めることにつらさを感じてるあなたに