レガシーなフロントエンドコードを整理するためにどう立ち向かったか

2エントリ連続でこんにちは、@mugi_unoです。

名古屋には台湾ラーメンイタリアンという名物があるそうです。

富山県民の私には理解が追いつきませんでした。


フロントエンドでの金額計算処理

さて、Misocaは請求書作成サービスなので、金額計算処理が欠かせません。

フロントエンドも例外ではなく、消費税額や合計額を算出するロジックが存在します。

機能変更が必要になった!!

諸事情により、そのロジックに変更を加える必要が生じました。

長くプロダクトを支えてくれていた存在ですが、内容的にはいわゆるレガシーなコードで、たびたび開発者ミーティングでも課題として挙げられることがありました。

git log で確認してみると、該当コードに対しての機能的な変更は2015年の冬から行われていません。

f:id:mugi1:20180907094553p:plain:w400

何が問題だったのか?

DOM操作と計算ロジックの混在

Misocaでは、新しくコードを書く際はVueやReactを使っていますが、まだまだjQueryで書かれたコードも残っています。今回の計算ロジックはそれが顕著で、100%jQueryで記述されていました。

参考としてこちらの図をご覧ください。

(画質は落としています)

f:id:mugi1:20180913131517p:plain:w100

これは、該当コードから一部を抜粋し、

  • ■緑 : DOMからの読み込み
  • ■黄 : 計算処理
  • ■赤 : DOMへの書き込み

でざっくりと色分けしたものです。カラフルで綺麗ですね!

さて、これはつまり、画面で見えている要素と計算処理が密接に絡み合ってる状態であることを示します。

今回の機能変更では、この中の 「■黄 : 計算処理」の変更が必要ですが、このまま挑んでも

  • 「計算処理を変えたつもりが、他の箇所でのDOMからの読み込みも変わっちゃった!」
  • 「DOM触ったら、全然違う場所のコードの計算処理も意図しない動きになっちゃった!」
  • 「つらいです」

となるのが容易に予想できます。

変更時の影響箇所が読めない

内部の複雑さだけではなく、該当コードはグローバルに定義されているため、あらゆるところから自由にコールされています。かつ、参考となる資料も特に存在していないため、変更時にどこまで影響が及ぶのかがわからない状態でした。

これによる最大の問題点は、コードを触るのが怖くなるという心理状態になることです。

修正をしても「関係のないところには影響がないです!」と誰も言うことができません。仮にPRを出したとしても、レビューする側は「ウッ...」となります。

まずはリファクタリングだ!

色々と大変なことはわかりましたが、嘆いてばかりでは何も改善しません。

もともとこのコードが作られた時期を考えると、jQuery全盛期であり、React/Vueといったものが世の中で活躍し始めるよりもずっと前の話ですので、むしろ今まで僕たちのプロダクトを支えてくれてありがとう、と言うべきなのかもしれません。

感謝の気持ちを込めて、リファクタリングを行うことにしました。

コードを知る

すぐにコードを書き直していきたくなるところですが、グッとこらえ、まずは誰が見ても分かる状態にコードを可視化することにしました。

というわけで、書いたのはこんな雰囲気の資料です。 draw.io で書きました。) f:id:mugi1:20180910163646p:plain

  • 緑色が読み込み専用のDOM
  • 赤色が書き込みも行うDOM
  • 線がデータの流れ

です。

資料書くとか無駄じゃない?という意見もあるかもしれませんが、

  • ある値が、一時的なものなのか、最終的な計算結果なのか
  • あるDOMを変えた場合、どこの書き込みに影響を及ぼすのか

を目に見える形で整理することで、コードを理解出来ると共に影響範囲が自分の中で整理できますし、リファクタリングする際にも様々な戦略を考えられるようになります。

また、レビューをしてもらう際にも、自信を持って「影響箇所はここです!」と言えるようになりますね。

リファクタリングの対象範囲を決める

一言にリファクタリングといっても、やみくもに始めると際限なく直してしまうので、 どこまでやるのか?も決めたほうがよいでしょう。

今回の目標は以下に設定しました

  1. 計算に必要なデータが、1箇所に集約できている
  2. DOMの変更を行う箇所が、1箇所に集約できている
  3. 計算処理が、DOM操作を伴わないJSの単一のfunctionになっている。

逆に、以下はやらないことにしました。

  • グローバル依存からの脱却
  • React/Vueといったフレームワークへの書き換え

「本来やりたい機能追加のためには、最低限ここまでやれば大丈夫だろう」というところまでを目標にしました。

怖い修正をする際には、小さく刻んでいくのも大事ですよね。

ちなみにこういった選択を出来たのは、最初にコードを可視化して整理したことで「どうやら小さくリファクタリングできそうだぞ?」ということが明確にわかったから、というのも大いにあります。

整理をしていなかったら、「全部Vueで書き換えるんじゃ〜〜、ウワァアア...」という未来が待っていたかもしれません。

テストコードを追加する

ここまでの整理をしても修正ミスをしてデグレを引き起こすのが人間です。

そんな時僕たちに自信をくれるのがテストコードです。Railsのfeature specによってある程度はカバーされていますが、いくつかの問題がありました。

  • 実行に時間がかかる(1分とか)
  • サービスのユーザ体験を担保しているものなので、網羅性は保証できない
  • リファクタリングした以外の箇所の問題とぶつかると永遠にハマる

特に実行時間がネックで、リファクタリング中は何十回・何百回と叩くことが予想されます。 そのたびに待たされていると作業ペースが落ちてしまうので、フロントエンドだけを対象としたテストコードを用意することにしました。

既存コードのInput/Outputのみにフォーカスしたテストを書く

さて、DOMにべったり依存しているコードですので、テストを書くと言っても一工夫必要です。

リファクタリングした結果、既存の挙動を壊していないことを担保したいので、

  • 該当のグローバル関数を実行する前のDOMをInput
  • 該当のグローバル関数を実行した後のDOMをOutput(期待結果)

として、ブラックボックステストを追加しました。

テストコードの例

影響のあるDOMのみを含む、最小限のテスト用テンプレートをpugで用意します。(先に資料化したものが役立ちました)

doctype html
html
  body
    //- Input
    input(
      name='quantity'
      value='1,000'
    )
    input(
      type='radio'
      name='tax_rate'
      checked
      value='8'
    )

    //- Output
    #total
    #sub-total
    #tax

...

テンプレートをロードして計算を行い、結果を確認します。

describe('計算処理', () => {
  beforeEach(() => {
    document.body.innerHTML = require('./test.pug')();
  });

  it('〜の計算結果が反映される', () => {
    window.run(); // リファクタリング対象のグローバル関数

    expect($('#total').text()).to.eq('10,800円');
    expect($('#sub-total').text()).to.eq('10,000円');
    expect($('#tax').text()).to.eq('税別 800円');
  });

...

最終的には、おおよそ200件ほどのexampleを書きました。

リファクタリング後に捨ててしまうことも視野に入れ、テストコードの綺麗さよりも、カバー範囲の広いテストを1件でも多く、時間をかけずに書くことに注力しました。

f:id:mugi1:20180911100127p:plain

テスト自体も3秒程度で終了します。いい感じですね!

小さく整理していく

ようやく準備が整いました。これで自信を持ってリファクタリングを始めることが出来ます。

リファクタリングする際は可能な範囲で小さい単位に区切りながら進めます。レビューする側の負担を軽減するという意味もありますし、そもそもリファクタリングの方向性が間違ってないか?というのを適宜確認できる効果もあります。

f:id:mugi1:20180912091111p:plain

ここまでやると、ある程度処理が分離された状態になってきます。

最後に、全体を通した整理を行い、ついでに不要な処理の削除やモジュール分割といったプラスαのリファクタリングを行いました。

f:id:mugi1:20180912092238p:plain

リファクタリングした結果どうなったか

リファクタリング前のコード同様に、 リファクタリング後のコードを抜粋してカラーリングしたものがこちらになります。

  • DOMからの取得処理

f:id:mugi1:20180913131930p:plain

  • DOMへの反映処理

f:id:mugi1:20180913131953p:plain

  • 計算処理

f:id:mugi1:20180913132012p:plain

  • グローバル関数本体

f:id:mugi1:20180913131837p:plain:w200

どこでなにをやってるかが明確で、綺麗になりましたね!

まとめ

今回はレガシーコードをリファクタリングしてみた話でした。

こういう作業には少なからず恐怖がつきまとうので、いかにしてそれを払拭しながら小さく進めていくかが重要だな〜と実感しました。

ただ、今回のリファクタリングである程度は綺麗になりましたが、正直なところ、まだまだ理想の状態には程遠いです。

と、いうわけで..

採用

Misocaでは、リファクタリングも楽しめるエンジニアを募集しています!