リファクタリングをしよう

こんにちは。
デザイン・システム室の森口です。

以前はweb・グラフィックデザインなどを行っていましたが、現在はエンジニア業務をメインで行なっています。
主にミールタイムに関わる案件を担当しており、最近はリファクタリングや業務改善につながる機能の作成を行いつつ、合間に不具合の調査や他部門から依頼されたデータの出力などを行っています。

その中でも、今回はどのようにしてリファクタリングを進めているかについて少しお話ししようと思います。

まず、リファクタリングの方針として以下の3点を念頭に改善を行なっています。

１. 役割を意識する
２. 不必要なコードは削除する
３. 完璧を目指さない

これは、Viewにロジックが普通に存在していたりControllerが2000行あったり重要なロジックが複数コントローラーにコピペで存在していたりと、長い年月をかけて徐々に形成されてきた化石を発掘しながらきれいに整理する作業を一人で行っているからです。

対象は山のようにありますから、完璧にこなそうと思うと終わりがありません。そうでなくても終わりはありませんが、次にリファクタリングする際により良くすれば良いのです。

ちなみに、テストコードは存在しません。

ロジックを整理する

複数コントローラにコピペ状態で存在するロジックや、同じような処理を行なっているが微妙に違うだけなロジックを役割毎に整理します。ロジックらが属するべきモデルにメソッドを生やしたり、新たなクラスやサービスクラスを作成します。

これらを行う際は、「微妙に違うだけなロジック」は本当に同じなのか、サービスクラスを新たに作るのは適しているのかを考えるようにしています。

「微妙に違うだけなロジック」だがほぼ同じようなロジックがコードは似ているが使われ方が全く異なる場合、これらを共通化してしまうとバグの温床になったりメンテナンスが行いづらくなります。実装の類似ではなく、意図の類似で共通化の判断を行うべきです。

ロジックを役割毎に分割するのはイメージしやすいと思いますが、サービスクラスを作成するのはどのような場合でしょうか。

ECサイト上で商品をカートへ入れる処理で例えてみます。

１. 商品情報が入力される
２. 販売可能なステータスであるかを確認
２-１. ステータスチェック
２-２. 在庫数チェック
３. カートへ入れる
４. 在庫の減算処理

上記の流れのうち、2,3,4は一連の処理として行って欲しい箇所です。そのような箇所をサービスクラスとして独立させ、その中で行う2,3,4の処理はモデルへ記述しています。

マジックナンバーを整理する

マジックナンバーというのは当事者にしか意図がわからない数字です。

status: 1,

上記のステータスが1の場合の意味はこれだけではわかりません。

status: 2 # 解約ステータス

コメントで書いてあれば推測もできますが…
ドキュメントやコメントなど補助するものが無い場合は、実際の運用実態から推測するしかありません。ちなみに社内にはドキュメントも存在しません。

意図がわからないコードというのはとてもメンテナンスが行いづらいものです。コードに限らず、何かを指示する際は意図を持って行うべきだと思います。なのでリファクタリングを行う際はこれらに意味を与えます。具体的には定数や変数を使った名付け整理を行います。

まとめ

今回紹介した内容はいずれも「該当のコードを書いた瞬間のみを意識して作成されたコード」に起因する問題であるということです。どのようなコードを書くにせよコミットしてマージされれば他の人に影響を及ぼしますし、プロジェクトが続く限り、そのコードが使用され続けるかもしれません。
自分以外が見てもわかりやすい書き方であるか、メンテナンスは行いやすいかを考えると良いと思います。