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箇所に集約できている
DOMの変更を行う箇所が、1箇所に集約できている
計算処理が、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円');
  });

...