リファクタリングのアンチパターン

前提

ソフトフェアを運用保守していく上で、継続的なリファクタリングは大切です。
一方で、「動いているコードには触るな」という不文律もあります。
これは対立する方針のように見えますが、たいていは不適切なリファクタリングを防ぐために言われることであり矛盾していません。

この記事では、ボーイスカウト・ルールを題材に不適切なリファクタリングについてまとめてみました。

ボーイスカウト・ルール

ボーイスカウト・ルールとは「来た時よりも美しく」というルールです。
ボーイスカウトがキャンプ場を利用した時に、たとえ自分たちが汚していなくてもキレイにしてから帰ることで、次の人達が気持ち良く過ごせるようにしよう、というものです。
ソフトフェア開発においては、機能追加やバグ修正でソースを触る際には、どこか少しでも良いので改善しようということになります。

これ自体はとても良いルールです。しかし、ルールの使い方を間違えると、薬が毒になるのと同じくソースが「改悪」されたり、開発に悪影響が出ることになります。

アンチパターン

何しに来たの？

キャンプに来たのに、ずっとキャンプ場を掃除しているのはどうでしょうか？
特にグループで来ている場合、みんながテントの設営やご飯の準備といった作業をしているのに、それをよそに掃除ばかりしていると、サボっているとか自分勝手な人と思われるかもしれません。

リファクタリングのチケットでもないのに、かなり広範囲なリファクタリングを行う人がいます。
チーム開発では、そういった合意のない過度のリファクタリングは問題となるかもしれません。
例えば、大量の修正の入ったPRが飛んできたレビュワーはどうでしょうか？修正範囲が広ければそれだけレビューの負担が増えますし、時間がかかります。あるコードの変更理由が、本来の修正なのかリファクタリングなのかリファクタリングに伴うミスなのかを判別するのはかなり面倒です（PRやコミットが明確に分離されていれば良いのですが、このパターンの場合は一緒にされてしまっていることが多いです）。また、修正によって大量のコンフリクトが発生し他の人の作業に影響が出るかもしれません。個人的な経験ですが、そこまで時間のかからないはずの改修に、ついででリファクタリングをいろいろ行う→バグが見つかる→修正範囲が広すぎて原因究明に時間がかかる→その間に仕様変更が入り、それに対する修正も実施→テストやレビューを一からやり直し→また、バグが見つかる・・・といった悪循環を何度か繰り返した結果、数ヶ月もかかったことがあります。

このように、過度のリファクタリングはスムーズな開発サイクルを妨げることがあります。スクラムなどである程度開発サイクルが機能している場合には、リファクタリングをそこそこに抑えて、その分たくさんのチケットをこなした方が最終的にはより多くの改善が行えます。
とはいえ、汚いコードを修正したくなるのはプログラマの習性ですし、特にコードを書ける人ほどついついやりがちです。もし、本来の修正よりリファクタリングの方のコードが多かったり時間をかけているなら、この罠にはまってしまっている可能性が高いです。
本来の目的は機能追加やバグ修正であることを忘れないようにしましょう。

最後まで責任を持たない

キャンプ場にゴミがたくさん落ちていたので、ゴミを拾い集めました。でも、そのゴミをちゃんと処分せずにそのまま集めた状態で放置すると、ゴミ集めは無駄な作業となります。それどころか、燃えやすいゴミを火を扱う場所の近くに置いたりままにすると危険な結果になることもあります。ゴミを集めるなら最後まで責任を持って処分しないといけません。

リファクタリングをした時に、デグレが起こっていないかを確認するまでが修正者の責任です。
どれだけ簡易な修正であっても、ビルドが通ってコードをレビューしただけでは不十分です。
例えば、リネームと言っても動的に名前を使って参照している箇所があるかもしれませんし、ケアレスミスで修正漏れがあるかもしれません。処理をメソッドにまとめるだけでも、実は実行される順番が大事でメソッドに切り出したことによって実行タイミングが変わりバグになるといったこともあります。極端と思うかもしれませんが、これらは実際に障害として起こったことのあるケースです。

それで、影響範囲を洗い出してちゃんとテストを行う、あるいは、単体テスト等で処理が変わっていないことを保証するまでがリファクタリングです。もし、影響範囲が大きすぎたり特定できない、テストが困難といった場合は、別に計画を立ててリファクタリングを行うべきです。テストもせずに問題ないと判断するのは、リファクタリングとは言えない無責任な行動です。

美的感覚の違い

自然の中にあるキャンプ場は、キレイさにおいて妥協が必要なことがあります。例えば、山の中なら枯れ葉や枝がたくさん落ちています。ある人は落ち葉は全部掃除したいと思うかもしれません。街中の自分の家の庭であれば、それは可能かもしれませんが、山の中でそうするのは現実的ではありません。

時々、リファクタリングと称してマイルールな修正を入れる人がいます。
例えば、他の箇所では一般的に使われている命名を読みにくいからと修正したり、ある処理をよりクールな（見栄えがしたり最適化された）書き方に修正するといった具合です。
それ自体はコーディングルールで決まっているのでない限り、各自の裁量に委ねられた実装です。しかし、リファクタリングという観点だとどうでしょうか？
クリーンなコードの特徴の一つは「他のプログラマから見て明白である」という点です。同じような目的の変数やメソッドが人によってバラバラな命名をされているのは可読性が悪いと言えます。チームがその言語に精通したベテランばかりで今後もずっとそれが維持されるなら、どれだけクールな書き方をしても問題ありませんが、そうでないならもっと一般的な書き方にしておいた方が生産性はあがります。
もちろん、他の箇所で一般的に使われていても、明らかに間違いで修正した方が良い場合もあります。そういった時は、コーディングルールを策定するなり、チームと話し合うなりしてから修正に取りかかるべきです。でないと、他の人はその書き方を続けてしまい、いつまで経っても修正が完了しないかもしれません。

ボーイスカウト・ルールには、「次の人達が気持ち良く過ごせるように」という目的があります。「自分が」ではありません。同じようにリファクタリングも自分ではなくチーム（今と将来）にとってどうか？という観点で考えましょう。

もう帰る時間ですが・・・

キャンプ場の撤収時間が決まっているのに、必要以上に掃除をしてその時間を守らないというのは、良いことでしょうか？
いいえ、仲間の帰宅時間が遅くなったり、次の利用客を待たせたりと迷惑にしかなりません。

ほとんどのプロジェクトではスケジュールや予算が決まっており、それは当然守るべきものです。
明示的なスケジュールがなくても、簡単なバグ修正に何日もかけるといった行為は良くありません。
修正が必要なバグには、その修正を待っているユーザがいます。それを忘れてはいけません。

それで、「リファクタリング」に時間を使い過ぎることがないように注意しましょう。
この「リファクタリング」には、単にコードを修正するだけでなく、修正後のテストまで含まれます。修正自体は簡単でも、影響範囲が大きくテスト工数を考えると時間が足りないといった場合、修正を見送るという判断をするのも大事な仕事です。

さっさと逃げろ

キャンプ場が想定外の大雨で浸水する危険が出てきた時に、丁寧に後片付けをするでしょうか？
そんなことをしていると、最悪人命にかかわります。一刻を争う場合は、道具をそのままにして逃げることも必要です。

プロジェクトでも、障害が起きていたり深刻なバグが発生した時は、優先度を正しく見極める必要があります。
「今」そのリファクタリングを行うことが障害対応に必要不可欠でしょうか？
ただでさえ障害対応時は焦りやすくミスが連続しやすいので慎重な修正と入念なテストが必要です。それでいて、修正にかけられる時間もリソースも限られています。

緊急事態が起こっているのに、修正自体はほんの数行で済むものを、変数名の修正やメソッドの切り出しをしたりして数十行レベルの修正にするのは、リスクと工数を増やすだけです。そこに時間を使うよりも、修正漏れや想定外のパターンがないかの調査や検証など障害対応に注力するほうが賢明です。

不法侵入

掃除に集中するあまりキャンプ場のエリアを超えてしまい、よその土地に入ってしまうとそれは不法侵入です。
いくら良い目的であったとしても、勝手に入るのはダメです。

プロジェクトでも理由はいろいろですが、「ここは触らないで」という箇所があるかもしれません。そこが直接の改修対象になっていないなら、リファクタリングとして勝手に修正するのは避けましょう。触るべきでない理由が、改修予定が無かったり、目立ったバグも無く安定して動いているといったことであれば、もっと優先度の高いところのリファクタリングに注力することができます。デグレが起きやすといった理由であれば、慎重に計画すべきでボーイスカウト・ルールでリファクタリングするのは危険です。この場合、そのうちバグ修正でその箇所を触ることになるはずなので、それに備えてテストを増やしておくなど、環境を整えることから始めた方が良いでしょう。

いずれにせよ、何かしら他からリファクタリングに制約がかかるような場合、それを無視するのではなく相手の事情を尊重しましょう。可能なら代替手段が取れないか探りましょう。制約下においてどこまで良い結果を残せるかが腕の見せ所とポジティブに考えると良いかもしれません。

まとめ

リファクタリングで見られるアンチパターンを幾つかあげてみました。
こういったパターンが続くと、やがてリファクタリングやそれに起因するバグによって、開発が遅れるようになります。そうなると今度は同じ開発者同士でも「動いているコードには触るな」という発言が出てきやすくなり、最悪、リファクタリングに対する信頼が損なわれます。
こういった事態にならないように、基礎的ですが次のことに気をつけましょう。

よく計画を立てる

まず、リファクタリングは事前に済ませましょう。テストがあるか確認し、ないなら、まずテストを作ってください。
リファクタリングしたくなったら、影響範囲や今後の開発にメリットがあるかを、実装する前に考えてください。
もし、実装中にリファクタリングできるポイントに気付いた場合は、その作業が終わってからリファクタリングします。

周囲とコミュニケーションをとる

リファクタリングはチームのためにするものなので、チームがどういうリファクタリングを求めているのかの認識を揃えておく必要があります。あらかじめ、よく方針についてみんなで話し合っておくだけでなく、必要に応じて方針を話せる場を作っておくなら、一貫した効率的なリファクタリングを行えます。
また、チームの状況も把握しておくのも大事です。そうすれば、余力がある時には多めにリファクタリングができるかもしれませんし、進捗が厳しいならリファクタリングは最低限にするといった調整が行えます。
もちろん、他のチームが関係するなら常日頃から連携しておくことも大切です。

リファクタリングは、開発業務の一環としてチーム全体で日常的に行うものです。だからこそ、チームで考えてチームで行動するということが大切なのです。