デッドコードがもたらす弊害
皆さんはこんな経験ありませんか？
この処理どこで呼ばれてるんだろう→ちょっと調査→呼ばれてないんかい！
似ているメソッドがあるけどどっちを使えば良いんだ。このメソッド使っていいのかな？
デッドコード(到達不能コード)は、存在しても決して実行されることのないコードを指します。
これらのコードがあると、使っていないとは分かっていても少し脳のメモリを割かれる感じがしますよね。
使っていないコードが残っているとファイルの行数が単純に増えて、ちょっと気も重くなります。
気分的な問題だけでなく、デッドコードを残しておくと、誤って呼び出されてしまってバグを引き起こしてしまうなど、残しておいて良いことはひとつもありません。
テストコードに関しても、使っていないメソッドをテストするのは時間の無駄ですよね。
不要なコードをお掃除しよう！
そこで私の所属していたチームではGemを使ってのリストアップをおこなってデッドコードのお掃除を進めました！
どんな感じで進めたかをざっくりとご紹介できればと思います。
不要コードの洗い出し
不要コードの候補を洗い出すために、今回はdebrideというGemを使用しました。
debrideは静的解析によって 「使用されていない可能性のあるメソッド一覧を取得する」 というGemになっています。
サンプルにもあるとおり、コマンド一つでお手軽に可能性のあるメソッドを抽出することができます。
% debride lib
These methods MIGHT not be called:
MyClass
good_method lib/some/file.rb:16
bad_method lib/some/file.rb:20
他にもcoverband, rubycritic などのGemも候補に上がっていたのですが、コードに変更を加えずにコマンド一つで気軽に実行できて、導入しやすそうだったので、debrideを選択するに至りました。
他社の事例を見ると、debride(静的解析) + okuribito_rails(動的解析)で抽出するということもやっていたのですが、本番環境に影響を与えずに、手っ取り早く始めるためにdebrideのみで候補を抽出するという方法を取りました。
後は単純に計測のために毎回呼ばれてオーバーヘッドによる影響が怖かった。(oneshot coverageとか検討しても良かったかも？)
とりあえずdebrideを試す
デフォルトの状態でdebrideを実行すると、routingで紐づけられているコントローラーのメソッドも未使用としてリストアップされてしまうため、意図的に候補から除外してあげる必要があります。
rails c してroutingに紐づくメソッドを下記で抽出しました。
routes = Rails.application.routes.routes
methods = []
routes.each do |route|
controller = route.defaults[:controller]
action = route.defaults[:action]
next unless controller && action
controller_class = "#{controller.camelize}Controller".constantize
if controller_class.method_defined?(action.to_sym)
methods << action.to_s
end
end
抽出したメソッドをwhitelist.txt に貼り付け。
optionでこちらのwhitelistを指定してあげることで、ルーティングに紐づくメソッドを除外した結果を取得できます。
debride --rails --whitelist whitelist.txt app
実行した結果149箇所、約2000行のコードが消せる可能性ありということが検出されました。
リストアップしてひたすら削除
これらの検出結果をリストアップして、後はチームで分担してせかせかとお掃除していきました。
使用されていない可能性のあるものが検出されているだけなので、再度該当箇所でGrepをかけて、本当に使用されていないかを実装者+レビュアー2人の3回チェックしてからリリースを行いました。
特に下記のケースに注意してました。
・after_commit などのコールバックで使用されていないか
・sendなどで呼び出されていないか
・ありがちなメソッド名。例えばsumという名前のメソッドみたいに、Grepすると複数箇所でてくるみたいなメソッドは簡単に判別するのが難しかったので、一旦消すのを避けました。
などなど
不要テストも併せて消せたので、結果として2153行を消すことができました！
感想
「問題なければ後で消す」「これは使わないでください」みたいなコメント付きの使用されていないコードがいくつかあって、当時は消すことがタスクに積まれていたけど段々と忘れられていったんだろうなという痕跡がいくつか見つかりました。「後で戻すかも。」みたいコメントついたコードも5年前とかだったりしたので容赦なく消しました。
リストアップ後の作業自体は簡単でサクサク行くのですが、やはりまとめてやると単純作業なので、ちょっとしんどい感があります。
案件で自分が触っているファイルだけでもdebrideをかけて、ボーイスカウト・ルールに則ってちょっとずつお掃除していった方が後々楽なんだろうなと思いました。
皆さんもぜひプロジェクトの隙間で手が空いたりしたらコードのお掃除をしてみてください！ちょっと気持ちがスッキリするかも...??
追記)クックパッドなんかは「お台場プロジェクト」という名前をつけて不要コードだったりGemの削除なんかを進めていたみたいです。
https://logmi.jp/tech/articles/319343