リファクタリングをする際にソースコードの設計からはじめてはいけない

リファクタリング フロントエンド 学び

どうも、レコメンド商品のシステム開発をしている野川と申します。 私は、2021年にモノタロウに新卒入社し、2022年5月からレコメンド商品の開発に関わり始めました。

モノタロウのレコメンド商品は、下の図の①～④の流れでクライアントサイドで表示しています。大部分の処理はJavaScriptで構成しており、UIもそのHTML部分をjQuery(JavaScript)で作成しています。

図：レコメンド商品表の流れ

入社当時私は、ソフトウェアエンジニアとして、「可読性の低いコードは駆逐するべきだ」「読みやすいコードだけが正義である」「理解しやすいシステムだけが皆を幸せにする」と心の底から考えていました。加えて、「なぜ先輩たちは可読性の低いコードを放置して平気なのか？」と疑問を持つこともしばしばありました。

レコメンド商品周りのコードはまさに可読性の低いコードベースとなっていたため、当事者となった私はこれを技術的負債だと考え、リファクタリングしていくことを決意し実行に移していきました。

主に以下のことを実施しました。

  • 巨大化してしまったJavaScriptファイルを複数のファイルに分割してディレクトリ構成から見直すこと
  • ES5を含む古い書式のJavaScriptをES6以降の書式にすること
  • TypeScriptに書き換えること
  • ユニットテスト、E2Eテストを整備すること
  • UI(HTML/CSS)をReactとCSS Modulesを用いてUIコンポーネント化すること

しかし、まったくもって予定通りに進まず、可読性の低いコードたちがなぜこれまで残り続けているのかを実体験として学ぶことになりました…。

経緯

技術的負債との出会い

前提として、レコメンド商品のシステムはこれまで明確なオーナーシップを持つことがなく、複数の開発者が複数の案件で開発してきているという経緯がありました。そして2022年5月に商品推薦チームが作られ、これまで社内で宙ぶらりんになっていたレコメンド商品のシステム、開発、運用に対するオーナーシップを持つことになりました。チーム人数は3人で、その中の1人はレコメンド商品にとても詳しい方でした。

あるとき私は最初のタスクをもらいました。それはシンプルに「レコメンド商品をクリックするとWebブラウザの同じタブで商品詳細ページに移動するので、これを別タブで移動するようにする」というものでした。HTML上のある要素のtarget属性の値を「self」から「blank」に変えるだけです。

しかし、いざ作業を進めるためにソースコードを確認しても、どこを書き換えればいいかまったくわからないのです。レコメンド商品の表示方法は簡潔に聞いてはいたものの、その処理の流れがソースコードからまったく読み取れないのです。

先輩からは「色々な意思決定のもとで現在のコードになっている」とのことでした。 個人的には「色々な意思決定ではなく妥協なのでは」と感じていました。

例えば以下のようなコードが実際に存在します。

  1. ネストが深い
// 例えば、下記のようにネストが4段、5段となっている箇所があります。

if (recommendApiResponse) { // レスポンスが取得できる場合
  ...
  if (recommendData) { // レコメンド商品が取得できた場合
    ...
    if (isSaleEvent) { // セールイベントが開催している場合
      ...
    } else { // 特別なイベントが開催していない場合
      ...
      if (isPopup) { // レコメンド商品をポップアップ表示する場合
        ...
      } else { // レコメンド商品を通常表示する場合
        ...
        if (isPc) { // PCでのアクセスの場合
          ...HTMLの作成処理など
        } else if (isSmartphone) { // スマートフォンでのアクセスの場合
          if (isApp) { // スマホアプリでのアクセスの場合
            ...HTMLの作成処理など
          }
          // スマホのブラウザでのアクセスの場合
          ...HTMLの作成処理など
        }
      }
    }
  } else { // レコメンド商品が取得できない場合
    ...
  }
} else { // レスポンスが取得できない場合
  ...
}

ガード節（アーリーリターン）を活用したり適宜関数に抽出することでネストが深くなりすぎないようにした方が良いです。設計が高度になりますが、ポリモーフィズムや状態マシンを活用してクラス分割するのもよいとされます。

  1. 同じ条件文を持ったswitch文が複数個所に散らばっていること
// このようなswitch文が5箇所以上に散らばっています。
// 加えて、pageNameは数十種類あります。

switch (pageName) {
  case 'TOP': // トップページの処理
    ...
  case 'SEARCH': // 検索ページの処理
    ...
  case 'BASKET': // バスケットページの処理
    ...
  case 'CHECKOUT': // 注文ページの処理
    ...
  case 'MYPAGE': // マイページの処理
    ...
  ...
}

同じ条件文はなるべく一か所に集約したほうがいいです。そのためにポリモーフィズムを活用したり、条件分岐をカプセル化するためのデザインパターン（コマンドパターン、ストラテジーパターン、ファクトリーパターンなど）を取り入れるのが良いと思います。

  1. UIが命令的に記述されていること
  2. レコメンド商品のHTMLは、JavaScriptのstring型、string型の配列で表現されている。
  3. たとえ提供するUIが同じでもHTMLの構成が若干異なり、その結果わずかに異なるHTMLを作る関数が乱立している。これにつられて中身が同じようなCSSファイルも複数ある（○○Page.cssや○○Sale.css、○○Discount.cssなど）。
// このような関数が数十個あります。絶妙にDOM構成が違ったりします。

// レコメンド商品のHTMLを作成する処理の例
function createRecommendHtml(productData, isTaxIncluded) {
  const htmlList = [];
  htmlList.push(`<div class="RecommendHeader">`);
  ...
  htmlList.push(`<span class="PriceInfo">`);
  if (isTaxIncluded) {
      htmlList.push(`<span class="TaxIncluded">${productData.taxPrice}</span>`);
      ...
  } else {
      htmlList.push(`<span class="TaxExcluded">${productData.price}</span>`);
    ...
  }
  htmlList.push(`</span>`);
  htmlList.push(`</div>`);
  
  return htmlList.join('');
}

// トップページ用にカスタムされていたり...
function createRecommendHtmlOnTop(productData, ...) { ... }

// セールイベント用にカスタムされていたり...
function createRecommendHtmlForSale(productData, ...) { ... }

// 割引対象の商品を考慮していたり...
function createRecommendHtmlForDiscount(productData, ...) { ... }

...

UIは宣言的に記述した方が良いですし、同じUIを提供しているならそれを作るHTML/CSSは共通化・コンポーネント化した方が良いです。

レコメンド商品を構成するスクリプトは、こういった処理が同じファイル内に延々と書かれている状況であり、可読性の低い巨大なソースコードとなっていることがわかりました…。

リファクタリングの決意

オーナーシップを持ち、日々技術的負債に悩まされ続けることになった結果、自他ともにあまたの不具合に苦しむこととなりました。ユーザーに迷惑をかけてしまう不具合には、例えば下記のものがありました。

  • トップページのレコメンド商品の商品画像が一部読み込まれなくなっていたこと
  • レコメンド商品を左右にスライドする矢印ボタンをクリックしても商品をスライドできなくなっていたこと

イメージ画像：商品画像が一部読み込めていないレコメンド商品

耐えかねた私はリファクタリングを決意したのでした。

やったこと

ここでは、リファクタリングをプロジェクトとして立ち上げるために行ったことを淡々と説明していきます。

現状分析と理想像の定義

ソースコードに関して、現状、困っていること、理想を下記のような表形式でまとめました。

表：レコメンド商品のフロントエンドシステムにおける現状と理想（例）

現状 困っていること 理想
1ファイルが大きく、同一ファイル内に業務ロジックとUIロジックが混在している ・認知負荷が高い
・修正（編集や削除）の判断がしづらい
・軽微なデザイン（HTMLやCSS）の修正でもJavaScriptファイルの修正が必要であり、デザイナーだけで完結しないことで余計な工数がかかる
※HTMLやCSSといった静的コンテンツのみの修正では、デザイナーさんだけでリリースを進めることも多い。 		役割・振る舞いごとに適切にファイル分け、モジュール分割がなされていること
※リリースに関与する作業者が最低限であること。例えば、デザインに関する実装はUI/UXだけで改修、リリースできる状態
重複コードが多い ・認知負荷が高い
・修正（編集や削除）の判断がしづらい
・ビルド効率が悪い 		重複コードが必要最小限であること
※許容される場合もある
書式が古い（ES5の書式が多分に残っている） 変数や関数のスコープ管理が難しく、挙動が推測しづらい 書式が新しい（ES6以降の書式になっている）
（同様の）条件分岐が多い ・認知負荷が高い
・修正（編集や削除）の判断がしづらい
・ABテストなどの条件分岐を追加せざるを得ない実装の場合、他の条件分岐との関係性を考慮するのが大変 		条件分岐の記述箇所が少ない
ユニットテスト/UIテストが皆無 ・関数やモジュールを（仮に作成したとしても）ブラックボックス化できず、手動テストの確認事項が増えてしまう
・回帰テストを自動で行うすべがない（⇒後続のリファクタリングがしづらく、品質の高いコードを書き続けることが困難） 		・業務ロジックに対してユニットテストが作成できている
・UIロジックに対してUIテストが作成できている
開発者が、短いサイクルで、単独で、手元で、回帰テストができること⇒後続の開発・リファクタリングにおける不具合や手戻りの確率を大幅に削減できる
続く… … …

ソースコード設計、マイルストーン設定

現状と理想のギャップを埋めるようなソースコード設計を行いました。

レコメンド商品のHTMLは、これまでは、HTMLとして意味を持つ文字列をstring型の値として生成し、jQueryでHTMLに変換して動的に画面上に挿入していました。 理想の状態では、レコメンド商品のUIをReact(×CSS ModulesでUIコンポーネント)化することにしました。モノタロウでは現在、ECサイトのフロントエンドをNext.js/Reactにリプレースするプロジェクトが動いています。なので、その流れに乗るためにReactを採用しました。

tech-blog.monotaro.com

設計を行った当初、社内ではReactやReactコンポーネントのスタイリングに関してノウハウが溜まっているとは言えない状況でした。そのため、いきなりUIコンポーネント化をするのは難しいと判断し、下記のように段階分けしました。

表：リファクタリングのマイルストーン

リファクタリングパート モダナイゼーションパート
2022年10月 - Step① - 業務ロジックとUIロジックを別ファイルに分割する 2023年5月 - Step① - UIロジックのUIテストを作成する
2022年11月 - Step② - 業務ロジックのユニットテストを作成する 2022年6月~7月 - Step② - UIロジックをReact化、CSS Modules化する
2022年12月 - Step③ - 業務ロジックをTypeScript化、モジュール関数化する -
2023年1月~2月 - Step④ - UIロジックをTypeScript化、モジュール関数化する -

※リファクタリングプロジェクトの中でシステムモダナイゼーションを行います。リファクタリングの域を超えてシステムリプレース要素も含んでいますが、ここでは両パートをまとめてリファクタリングと呼びます。

ソースコード設計とマイルストーンをもとに関係者から合意を得られ、プロジェクトのひとつとして動けるようになりました。

リファクタリングの実施

モノタロウは開発組織が大きく、多くの開発者が並行して業務を進めています。そのため、自分の書いたコードがたくさんの開発者に読まれたり、使われたり、時に互いの修正箇所でコンフリクトが起きたりします。

そこで私は、下記を意識してリファクタリングを進めました。

  • 読み手の認知負荷を上げないよう、あるルールに沿ってコードの書式を修正すること
  • テストやリリースといった開発終盤に差し掛かる前にコンフリクトを検知すること（検知した場合は迅速に関係者に連携すること）
  • 意識に頼ることなくリファクタリングによるデグレードを検知、回避すること

これらについて何かしら仕組みを導入する工夫をしました。具体的な内容を紹介します。

  • コーディングルールを作成
    • 社内にすでにあるTypeScriptのコーディング規約に従いつつ、下記を厳密に満たすこと
      • 関数はアロー関数で宣言すること
      • 変数、関数はファイル外から参照されるもののみをnamed exportすること
      • 関数には必ず冪等性を持たせること
      • ユニットテストを作成すること
      • できる限りjQueryは使用しないこと（JavaScript標準のDOM APIに置き換えることを努力目標としました）
      • 変数の初期化にnullを使わないこと（TS開発コミュニティに合わせてundefinedを採用～Coding guidelines · microsoft/TypeScript Wiki · GitHub～）
  • 日次でCommit内容を監視
    • レコメンド商品を構成する主要なJavaScript/TypeScriptファイルに対する変更を検知するため、それらを触っているリモートブランチを特定するShellスクリプトを日次で実行すること（その後、リモートブランチから実装者や案件の資料、Pull Requestなどをたどって関係者を特定し、お互いのリリースの壁を取り払うような調整を都度行うこと）
  • レコメンド商品専用のテスト項目書の作成
    • ECサイト開発全体で使われているテスト項目書があり、それをレコメンド商品用にカスタマイズする
    • リファクタリング作業を進めるとともに拡充する運用にする
    • 項目のうち自動化できるものは都度ユニットテストやUIテストに落とし込む

このようにリファクタリングを進め、ちょうど1年が経過しました。マイルストーンのうちリファクタリングパートは完了しており、モダナイゼーションパートが進行中です。その過程で下記の実績を得ることができています。

  • リファクタリングした業務ロジックに対するユニットテストのカバレッジが100%
  • レコメンド商品専用のテスト項目書、それをメンテナンスするチームの運用体制
  • 6ヵ月間の不具合件数を6件(2022/5/1~2022/10/31)から1件(2022/11/1~2023/4/30)に削減できたこと

学んだこと、つまづいたこと

当たり前ですが、商品推薦チームができたおかげで、それ以前に比べてレコメンド機能を触る開発者が減りました。それによりチーム外でレコメンド商品を触ることが減り、リファクタリング作業中にコンフリクトが思ったより起きませんでした。

とはいえプロジェクトとしてはまったく順調ではなく、マイルストーン通り進んでいません…。 予定では、2023年7月中にリファクタリングが完了しているはずが、まだ50%程度残っている状態です。なんてこったい。

理由は明確で既存のバグなのか？隠れた仕様なのか？誰も答えを持っていない挙動がたくさん見つかり、仕様を改めて定義しなおすことが遅れた要因でした。 社内文書を漁ったり社歴の長い先輩に質問したり、プロデューサーに判断を仰いだり、ソースコードよりも人と向き合う時間の方が長かったのではないかと感じます…。

また、ソースコード設計の当事者は、もちろんその設計や思想を理解しているが、これを他者に共有するのが大変だったことの1つの理由でした。設計をTypeScriptのコードに落とし込むのが難しかったり、リファクタリングをリファクタリングしたり…。遅れがちになった結果、他の施策との兼ね合いでリファクタリングのリリースが延期になったり…。まったくもって思うように進まないもどかしさを感じました。

リファクタリングを実施して学んだことは想像よりも膨大なコミュニケーションコストが発生することでした。

リファクタリングのあるべき姿

商品推薦チームに入り、レコメンド商品にオーナーシップを持ちました。その途端、「こいつの面倒は俺が見る！」と責任感を自覚しました。

「今後長く付き合うシステムならば、品質高くありたい、簡単に修正できるようであってほしい。」

そう考えて長期目線での開発速度を高めるために短期集中でサクッとリファクタリングしてしまおうと思ってました。 ところが、先に触れたような下記の問題が起き、長期で取り組むことになりました。

  • 現行仕様が明確になっておらず、それを明確化するところに立ち戻る必要があること
  • 設計思想を共有しにくく、リファクタリング作業を進めること自体の認知負荷が高い
  • リファクタリング中に、より優先度の高い案件が入ることでリファクタリングが延期してしまうこと

延期の度にタスクスイッチングが起き、コミュニケーションコストが増える。 再度着手できるようになった頃には実装内容を忘れてしまっており、思い出すのにも時間がかかる。 延期することなく順調にリファクタリングできそうと思っているとよくわからない仕様に直面する。 やっとリファクタリング内容をリリースしたと思ったら既存のバグが顕在化してリリースをロールバックする。

歯がゆい思いを何度も何度もしながらリファクタリングをしているあるとき、下記の記事に出会いました。

qiita.com

あぁ！！このコードむかつく！！これ直しましょう！！

は大体死亡フラグです。糞コードが糞コードたる所以で、大体、1日、2日で直すことができないから糞コードなのです。こういう怒りに任せて、糞コードを若さのパワーで押し切るのは、大体みんな通ってきた道です。そして、日がたつにつれて、目は死んでいき、糞コードに目を向けるのが辛くなる。しかし、啖呵切った手前、直さないわけにはいかなくなり、精神的な重圧が強くなってくる。

技術負債を解消するうえでどういう力学が働くのか、なぜ技術負債が発生してしまったのかを考え、やるべきことを1つずつ積んでいくのが大事

糞コードは直すべきです。しかし、安直に直すな。

まるで自分のことを言われているようでした。マイルストーン通り進まずもがき苦しんでいた時にこの記事を目にしたので、とても胸に刺さりました。 自分の至らなさを簡潔に表現していると感じ、涙が出そうになりました。リファクタリングの難しさ、仕様を細部まで把握することの重要性を経験とともに学びました。

技術的負債の背景、根本原因

リファクタリングを始めてから知ったのですが、モノタロウのレコメンド商品は2013年ごろから歴史があるらしく、内製システム（～2014年頃）⇒社外製システム（～2015年）⇒内製システム（現在）といった変遷をたどっています。 この歴史の中で、会社もソフトウェアエンジニア組織も大きく変わっていて、それによりオーナーシップが宙ぶらりんな状態で何年も経過していたのです。

結果、様々なグループ・チームの開発者、案件でレコメンド商品を触っていて、それによって、仕様が記録されないまま忘れ去られて、また変更されてを繰り返した結果、ソースコードがスパゲッティ化して、何が正解なのかわからなくなってしまったようでした…🍝 （オーナーシップのない仕事なんてさっさと終わらせたいですもんね。気持ちはお察しします。）

「妥協」ではなく「色々な意思決定」の積み重ねがシステムの今を構成していて、それがお金を生み出してきたと同時に技術的負債を生み出すものであることがわかりました。

リファクタリングをどう進めるべきだった？

私は設計の際に、過去の経緯やビジネス判断の積み重なりによる変化とあるべき姿を考慮できていませんでした。すなわち歴史的背景を理解せずにソースコード設計、マイルストーン設定をしていたことに気づきました。リファクタリング対象の全貌を把握しないままリファクタリングを始めた事が本質的な原因だと考えました。 （恥ずかしながら、目的地を定めずに出発していたということです。）

あらためて整理すると、設計のタイミングでは下記の太字部分を行うことが大事でした。

表：リファクタリングのマイルストーン（改善案）

現行仕様の把握 リファクタリングパート モダナイゼーションパート
各機能ごとの外部設計 Step①：業務ロジックとUIロジックを別ファイルに分割する Step①：UIロジックのUIテストを作成する
歴史的経緯の把握と現行の機能設計 Step②：業務ロジックのユニットテストを作成する Step②：UIロジックをReact化、CSS Modules化する
- Step③：業務ロジックをTypeScript化、モジュール関数化する -
- Step④：UIロジックをTypeScript化、モジュール関数化する -

とはいえ、計画的に進まなかったのみで、具体的な成果はありました。

  • テストコードを用意できたこと
  • 仕様をテスト項目書として明文化できたこと
  • 同じUI/UXを提供する場合にそのソースコードを共通化できたこと
  • 不具合件数を減らしたこと
  • 浮き彫りとなって現状と合わない仕様・機能を見つけ、それらを修正できたこと
  • リファクタリング箇所のソースコードが他チームの案件の参考となっていること

などなど。

そして何より教訓を得ました。

「ビジネス観点で正解を定めずに進むと痛ーい目を見る」

言葉としては当たり前ですが、この当たり前を痛感する経験をしました。 巷でよく言われるリファクタリングは、ある程度正解はできており（≒仕様が明確で）その検証をするといった印象です。それに対して今回は、誰も正解（仕様）のわからないシステムに対しては、一般的なリファクタリングの考え方だけでは太刀打ちできないものでした。

所感

完了を予定していた2023年の7月を過ぎた今でもリファクタリングは継続中です。あとは粛々とモダナイゼーションパートの作業を進めるだけの状態です。

継続するにあたってチームメンバーが協力的であったことで本当に救われました。チームメンバー全員で協力して実装、レビュー、テスト、リリースを行ったことでメンバー全員が当事者意識を持つことができ、リファクタリング作業をどうやったら効率的に進められるかといった観点での議論が活性化しました。また、いきなり理想の状態へソースコードを持って行かずにパート毎、ステップ毎に順を追って進めたこともよかったと感じています。1つのPull Requestで対応する範囲が明確になり、また複数のPull Requestのリリースを重ねていくことで達成感が得やすかった印象です。

今回、レコメンド商品を一から作り直す道も考えられましたが、そうしませんでした。というのも、既存のシステムでしっかりお金を稼いでいたからです。そのため、それを作るソースコードをすべて破棄するのではなく、ステップを踏んでリファクタリングを重ねることでリスクを抑えてリファクタリングを進めて行くことにしました。 先のQiitaの記事にもありましたが、「金を稼ぐ技術的負債に敬意を払うべし」はもっともだと私は思います。敬意を払って過去の経緯をしっかり追い、仕様を明確にした上で「技術的負債」と呼ぶべきです。 もし仮に一から作り直していたとしても、結局仕様がわからないものに悩まされてマイルストーン通りに進められなかったと感じます…。

時間が経つにつれて技術的負債がたまっていくのは仕方ありません。システムにオーナーシップを持たせることで、技術的負債を解消する自浄作用をある程度期待できます。 しかし、技術的負債の解消、リファクタリングは簡単な道ではないと強くお伝えします。具体的な設計やマイルストーン、協力し合う仲間、そして何よりリファクタリング対象への理解があってこそ進められるものです。これらに自信をもって「Yes！」と答えられないならば、技術的負債を受け入れましょう。

最後に、技術的負債の解消、リファクタリングをしたい人へ。 モノタロウのシステムには歴史があり、モダンな部分とそうでない部分が混在しています。リファクタリング、モダナイゼーションしがいのあるシステムがたっぷりです。もしご興味がありましたらカジュアル面談にお越しください！

hrmos.co