はじめに
「糞コード」について、すっごい盛り上がっちゃったエントリーがあって、それぞれ言いたいことは判るけど、あんまり建設的では無い方向になっているなぁと思う。
では、建設的な方向とはどのようなものかというと、私の場合こうやって凌いでます的なことではないかと思い、私の場合を書くことにする。
当然いろいろな意見の中の一つなので、意見もらえればへぇと思い参考にしたりしなかったりします。
基本方針
基本的にリファクタリングに興味がなく、「動いているものを直すな」という文化での糞コードの直し方の話とする。また、レビュアーや上司はコードがある程度読める前提である。
リファクタリングに理解があるなら、どんどん直せばよいだろう。コードを読めなければ、全部の責任はあなたが被るという意味なので、勝手にやればよいだろう。(そうしたこともある。)
糞コードに罪なし
糞コードが書かれる背景には様々な背景がある。あなたもコードを日常的に書いているなら糞コードを書きなおかつ放置したことが無いとは言わないだろう。
- 時間が無い
- スキルがない
- 度重なる仕様追加
- 矛盾した仕様追加
- スキルが無いのにレビューする上司
- ユニットテストが無い
- リファクタリングへの無理解
- 眠かった(忙しすぎる)
しかし、直せるときに直せなかったらそれはあなたの罪だろう。報告していれば、直せるタイミングは増えるだろう。
黙ってやる
でも、実際は糞コードの指摘はするべきでは無い。前項にある通り糞コードをそのものは罪ではないはずなのだが、あとからそれをわざわざ指摘するのは「後知恵」で意地の悪い行為とみなされることが多い。
と同時に、レビュアーやマネージャには問題が見えていないことが多い。その場合、幻覚でも見ているのでは無いかと言いたげな反応をされることになる。ここで声高になることは互いの人格を傷つけあうのと同じになる。(という経験をたくさんした・・・)
大抵の場合は「具体例を出せ」ということになるが、半端な具体例ではツッコミを入れられるだけだしそのための工数を用意してくれるマネージャがいた試しが無い。ただし、具体的なコードがあれば納得してくれることが多いのも事実だ。
チームの文化にもよるが経験的には、糞コードの指摘はしない方が良い。(これが糞コードによるバグの頻出であれば別。問題意識の共有ができているので、どのような仕組みがバグを呼び込んでいるか説明すれば良い。)
しかし、勝手にやらない
リファクタリングに理解があるチームにいるのならこんな文章を読んではいないだろう。と同時に糞コードを読まされるのも、それによって作ってしまったバグの責任を取らされるのはあなた自身ではないかと思う。
だまって、工数をかけて修正を行うと折角のコードは認められず、しかも自分の評価を下げることになるだろう。(土日をかけて修正したコードをブランチごと消せと言われた経験があります。)
現行のワークフローから逸脱するのはおすすめできない。
まとめると
- 放置すると自分に害がある
- 許可はもらえない
- 勝手に工数はかけられない
八方塞がりのように思えるがこの範囲でもできることがある。
ひっくり返すと
- タスクの範囲内で修正する
- レビュー時に具体的なコードで改善例を示す
- テストがあり、確実にバグが減る
この範囲であれば基本的にどのチームであれ変更が拒否されることは無いと思う。(古いコードをコメントアウトで残す文化がある場合は無理かもしれないが・・・)
大事なのは官僚主義的な組織が納得するよう実績ベースの改善を行うことだ。
個別の糞コード
影響範囲が大きいコードではなく、影響範囲が判っているコードの場合について書く。別に影響範囲が大きくてもそれほど変わらないんだけど説明が簡単になる。
きっちり調査
先ず、影響範囲が本当に個別のものなのかしっかり調べる。特に以下のものは注意したい。(言語や環境によっては他もありそう)
- マクロ
- プリコンパイラ
- リフレクション
- マジックメソッド
- コード生成
ここらへんは意図しない方向に影響しがちなので、そもそも利用は避けた方が良いがその分、広範囲に効果があるので一概に否定できない。この点に不安があるウチはリファクタリングの範囲は確実な範囲にとどめておくべきだろう。
と同時にきっかり調べたつもりでこれらのものにあたった際は、それ自体が糞コードの一種だと思ったほうが良い。生贄になったと思い諦め、後の人のためにドキュメントかなにか残そう。
個人的には上記のようなものを使った場合、フレームワーク化して、コードも別管理にした上で新しく入ったメンバーが読むドキュメントを作成すべきだろう。
きっちりテスト
糞コードというのは概してテストがしづらい。リファクタリングはテストを容易にするため、テストのためにリファクタリングしたとなれば変更の理由となる。
その上、きっちりテストしたコードを破棄して、複雑なコードと複雑なテスト見たがるレビュアーは多くない。
なにより、充分なテストはそれでも拾い残したバグの言い訳に使える。それで、免罪というわけには行かないが、リファクタリングがなければ他のバグを作ったはずだと主張できない訳では無い。（弱っ）
ほどほどのリファクタリング
技術的負債の利子はコードの変更時だけではなく、読む場合も払うし、読むことの方が機会としては多いことは能く知られている。
しかしながら、元々のユニットテストがよほどしっかり作ってなければ、リファクタリングはほどほどが良い。
理由二つありは、バグを作り込んでしまえば無理なリファクタリングをしたとみなされるから。「動いているコードを直すな」に拍車をかけてはならない。
もう一つは、差分が大きすぎるとレビュアーの負荷が重くなりすぎるから。
またこれは別の視点だが、個別の修正でかけられる工数以上のリファクタリングを行ってはならない。糞コードの場合、リファクタリングをしながらだとそうしないより、それなりに作業に余裕ができるので妥当な時間で作業できる。
きっちりメソッド化
メソッドの抽出は、リファクタリングの基本の一つ。テストを容易にして、見通しを良くする。何より、さらなるリファクタリングの足がかりになりる。
最低限、変更点は小さなメソッドの内部になるようにしたい。
適切なワークフロー
リファクタリング中は、おまけで直したいあれやこれやが多いと思う。
- インデントの不備など、無関係でも修正すれば喜ばれるもの
- メソッド名の軽微な変更など、目こぼして貰えそうなもの
- 今回の変更点とは関係ない上記以外の変更
いずれの種類なのか見極める必要がある。通常のワークフローを逸脱したものは、どれくらい謝らなければならないのかを考えて腹をくくる必要がある。(つまりおすすめできない)
最小限の説明
どうしても、自分のコードを自慢したくなるものだと思う(私だけ？)が、レビューを通すことを最優先しよう。現行の問題点を指摘するのはこれまで放置していたレビュアーを攻めるのと同じと心得よう。(指摘と人格は切り離すべきだが、それができているならこの文章は読んで無いでしょう)
経験的にはレビュアーがリファクタリングに目覚めて、だったらここも修正してって言うこともある。そういう時はしめしめと思うようにしたい。(是非、できれば、そうしたい。)
広範囲の糞コード
上記の糞コードに対して、広範囲に同様のコードが散逸していたり、汎用のコードのインターフェイスに問題があるなどして、一定以上の工数がかかる場合はどうするという点について書く。
基本は個別で修正
一気に影響範囲すべてを修正するとレビュアーの責任範囲を超えるため、許可されづらい。
個別の修正の際にメソッド化したり、悪いインターフェイスをラップして使い勝手が良いインターフェイスに変更しよう。もちろん、個別の修正としての言い訳が立つ範囲でだ。
テストを保管
ユニットテストがあろうがなかろうが、一度テストしたならいつでも同じテストができるようにデータや実行したスクリプトを詳細に保管する。範囲はユニットテストの範囲で問題無い。その場合であれば、テストデータの容量というのは非常に限られている。
しれっと汎化
上記のように個別で修正したものは、別モジュールの同様の変更タスクを振られることは比較的多い。このとき、前回作ったメソッドを汎用的に使えるように修正しよう。
前回のテストケースがまるごとテストしたなら、汎化したとしてもレビュアーが「動いているものを直すな」とは言いづらいはずだ。
効果の主張
２回目の適用までは、効果の主張をするべきでは無いと思う。新しくしていることを悟られることはあまり良い結果を産まないからだ。
ただし、２回めの適用が済んだ後であれば、他のコードは適用漏れになっていると主張できる。充分な実績があるので、変更する事自体は否定されないと考える。
一気に適用
ここまで来たら全体への適用をするべきときだが、マネージャがそのタスクを実行するかどうかは判らない。ただ、問題を指摘し、改善案を示し、妥当性を証明した。ここまでやれたら、あとはマネージャの判断しか無く、できることはタイミングを見て催促することくらいだと考える。
おわりに（終わらない)
今回の説明は、本当に限られた範囲のリファクタリングをリファクタリングという言葉を使わずにゆっくりと行う方法である。この範囲を超えるものは改善できないが、何もできないということではないと思っている。
上手く行けばリファクタリングの文化が根付くのではないかと思うが、そう簡単に行った試しは無い。