デッドコードを消す中で見えてくるものもあるかも

はじめに

株式会社ウェイブでCoolmicのエンジニアをしている布施です。
Coolmicは日本のコミックを海外向けに配信するWEBサービスです。

今デッドコードを削除しまくっています。
その中でクラス設計などについて改めて思ったことを書きました。
あと以前読んだTidy First?の内容も思い出したのでそれについても少し触れています。

サンプルコードはRuby(Rails)です。

概要

• やっぱり責務を意識してモジュールを分割するのは大事
• テストもどの責務に対応するものなのか意識するのは大事
• 定期的にリファクタリングするのは大事

背景

これまで何度も決済処理周辺の変更が行われてきましたが、デグレを恐れて一部の古いコードを残したままにしがちでした。
プロダクトにおいて最重要機能の一つであり何かあったら困るので、リファクタリングするのも気が進まなかったのですが、取り返しのつかないことになっても困るので今回気合を入れて整理を始めました。

（設定）

開発・運用しているプロダクトにおいて、決済代行サービスAからサービスBの利用に切り替えたが、サービスBにトラブルが発生した時のためにしばらくサービスAのロジックを残しておき、一定期間が経過したのでサービスAのロジックを削除することにした。
というようなことがありました。フィクションを含みます。

気づき

そもそも消しづらい

削除対象のロジックが一部に集約されていれば対応は簡単ですが、そうではない部分もあります。
以下は例。

class SettlementsController < ActionController::Base
  def create
    # 省略
    settlement = if use_a?
                   ASettlement.new
                 else
                   BSettlement.new
                 end

    result = settlement.execute

    if use_a? && result.error_code.present?
      # 省略
    end
    # 省略
  end
end

このコードだと、サービスAとBそれぞれに固有なロジックがASettlementとBSettlementに抽出されているように見えますが、なぜか後半でサービスAに固有のエラー処理がコントローラのメソッドに直接記述されています。
実装中にデバッグのために一時的にこの書き方をしたものが結局修正されなかったり、納期が迫っていてとりあえず早く確実にロジックを入れる必要があったりしたのかもしれません。
このように、整理されていない条件分岐とともにデッドコードがあちこちに分散していると、安全確認に余計なリソースを使ってしまいます。

この例だと、サービスAに固有のエラー処理がASettlementの方に実装されていたり、そもそもサービスAとBで別のコントローラが実装されていたりすれば、責務がはっきりして保守しやすかったかもしれません。

どのタイミングで場合分けするのが正解なのか...

前節の内容の続きです。
パラメータに依存する固有の処理が一部しかなくて共通の処理が多い場合は、一つの共通のクラス（メソッド、関数など）を使い、その中で場合分けをするのが良いのでしょうか？
それとも、取りうるパラメータに対応したクラスをそれぞれ使い分けるのが良いのでしょうか？

前節の例では、「決済処理を行う」ことは共通しているので共通のコントローラを使用しており、「どの外部サービスを使用するか」は与えられるパラメータにより変わるので、外部にリクエストを送る処理は別のクラスに切り出されていたのだろうなと考えられます。
共通処理と固有処理の境界が明確に定義できている、かつ外部サービス用クラスのインターフェースを合わせられるならこの分け方でも良いかもしれません。
しかし、将来的に導入予定の外部サービスが独自のインターフェースを持っていたら、この場合分けに追加するのは変な結合を生みそうです。

コントローラのレベルで分けることも考えられます。
この場合は、逆に共通処理を別のクラスに切り出し、各外部サービス専用のコントローラからそれらを呼び出す形になるんですかね。誰か教えてください。

なにこの図？

不要に見えて必要だったテスト

class SettlementsControllerTest < ActionDispatch::IntegrationTest
  test "決済成功" do
    # サービスAを使う
    set_a

    assert_difference("Hoge.count", 1) do
      post settlement_url, params: { hoge: 'fuga' }
      assert aaaa
    end
  end
end

一つの決済手段でしかテストが実装されておらず、set_aだけを見てそのテストケースを消してしまいそうになりましたが、なんとか気づいて書き換えました。
実際にassertionしているところには決済手段に関係ないものもあったので、その部分だけを抽出して、共通処理と固有処理をそれぞれ単体テストにできていればよかったです。
決済手段に依存する部分も含めて統合テストとして書くなら、全ケースサボらず書かないと本質的ではなさそうでした。今回の場合は。

最後に

デッドコードの削除はリファクタリングの中でも単純な方だと思いますが、消す前にその周囲の処理内容や、なぜ実装当時はそこにそのコードが書かれたのかということを考えると、今後の設計に役立つ知見が得られるかもしれません。過去のコードに敬意を。

また、一度に大量のコードを消すのは安全が確認できていてもやっぱり怖いので、こまめにリファクタリングしておいた方がいいと思います。小さいバッチサイズで進めれば、リファクタリングに対するレビューコストもかからないし、作業している間に他の開発者の変更とコンフリクトするリスクも減ります。ってえらい人が言っていました。

テストもちゃんと整理します。

参考

https://www.oreilly.co.jp/books/9784814400911/