#はじめに
日頃、巨大な業務アプリケーションのコードを扱っています。
10～20年選手のコードが生き残っており、とある１つの計算処理単位中の分岐数が2万超、ドキュメントもテストも十分ではないレガシーだが現役のコードたちです。
感覚的なものですが、同僚による"ジェンガで出来たサグラダファミリア"は言い得て妙だなと思っております。
今回はそんな繊細なバランスで成り立っているコード群に、不具合修正を行うときにレビュワーとして気を付けていることを５点に絞って紹介します。
なお今回はJavaを中心に話をしています。
第一条 何が何でもノンデグ担保
デグレードしていないことがスーパーウルトラハイパー大事。
修正が不十分であることよりも優先。
必要十分な修正以外が入っていないか。
if文追加した範囲が大き過ぎるとか、returnが追加されている場合など、要注意。
修正したいケース以外の所まで仕様が変わっていないだろうか。
ついでに修正しちゃいたい気持ちになったとしても、分からないなら触るべからず。触らぬ神に祟りなし。
触りたいなら、テストせよ。絶対にだ。
たぶん大丈夫なんてものは存在しない。確認した事実こそが全て。
【理由】
余計なトラブルを回避しよう。今のまま良い奇跡のバランスが取れている可能性がある。
どうしても変えたいなら、変更の影響がないか、がんばって調べる＆影響が出る先との調整が必要なのだ。
それだけの覚悟はお持ちか。
そのネゴシエーションコストを払うのは、あなたではないかもしれない。
胸に手を当てて考えてみよう。今はその時だろうか。
第二条 スコープ！スコープ！！スコープ！！！
privateでいいならprivateにしておこう。
publicである必要性がないならpublicにしないこと。
JUnitで呼び出したいからpublicにしてるって？ それならpackage privateでいい。
【理由】
自ら影響範囲を広げるリスクをおかしてはいけない。
後々に、どこから参照されているか調べるコストを増やす。
同様の理由で、下手にリフレクションして呼び出すとかしない方がいい。
場所と使いどころにもよるが、後々の保守にあたるエンジニアたちの影響範囲調査が大変になり過ぎるような実装は負債になる。
また過度な共通化もしない方がよい。
コンテキストの差異が後々判明してきて、共通化の中に様々な例外ケースを入れないといけないことになったりする。
後からちょっと手を入れるために、調査・テストしなくてはいけない範囲が膨大になる。
社内製の共通ライブラリみたいなものは、一度できてしまうと、顧客からは機能強化のニーズが出てきにくい部分なので保守は手薄になってしまうことがある。
後々、誰が担当するか問題やら、誰も分かる人がいない問題が起きたりする。
第三条 周囲との調和
ハーモニー。その世界の文化に馴染むコードを書く。
メソッド内⇒クラス⇒パッケージ/jar単位⇒機能のサブシステム単位⇒製品単位
の順で、郷に入らば郷に従おう。
どうしても嫌なら、全部、書き直そう。全部直してテストできないなら、周りに合わせておくべし。
コメントと実装が乖離している？
実装とコメントが素敵な二重奏を奏で続けるために、コメントも修正しよう。
【理由】
後から読んだときに、理解に余計なコストがかからない。
（例：同クラス内でsya_idが急にemployee_idになっていたら、戸惑う。
一緒の概念なのか逐一確認が必要になる。例え、中途半端な日本語で微妙な命名と思っていようがsya_idを使おう）
後々に修正入れる際に、どちらに合わせるべきか、迷わせなくて済む。
規則性があると類似調査がしやすい。
第四条 論理的整合性
不具合事象、原因、原因への対処方法、その対処を選択した理由、影響範囲の認識。
これをレビュイーが語れるか。論理に破綻がないか。さらに、修正しているコードの間でも、整合性が取れているか。
ソースコードレビュー時には、一行ずつ、なぜこれが必要なのか、これでいい理由は？を考える。
分からないことは聞いてよい。
レビュワーがすべてを分かっているべきだから、質問するのは恥だ・・・なんてことはない。
無論、同じ質問をし続けないように学ぶ必要はある。
しかし疑問はそのままにしない方が、総合的に考えたら合理的である。
レビュイーと一緒に調べたっていい。あるいは、分かるひとに協力を依頼するのも良いかもしれない。
とにかく、ん？なんか変だな？っていう感覚をそのままにしないこと。
【理由】
１．レビュイーのバイアス
自分自身がソースを書いていると、動いた！できた！完璧！！という気持ちになる。
特に取り組んでいるのが、複雑なジェンガだと、難問を解消したときの喜びは大きい。
それが故に、ここで陥る正常性バイアスは非常に強力で、自力で抜け出すのが非常に困難である。
完全に忘れるまで寝かせておくなどすれば気づくかもしれないのだが、そんな暇はない。
他人に説明したり、質問されたりする中で、初めて、これひょっとして破綻してるんじゃ？と気付くことは多々ある。
２．可読性
よくわからないこと、は他の人にも分からない可能性が高い。つまり後から理解するのが難しい。
そういうコードは長く保守していく上で、リスクになる。
第五条 1度に1つずつ
One by one, Step by step
複数目的の大量の修正を1つのcommitに詰め込み過ぎない。
小さめに意味のある単位でcommitを分ける。
またMRの単位でも不具合毎に分けるべし。
【理由】
論理的整合性を確認する上でも、とても有用。大きすぎると覚えていられない。
また、何らかの上手くいかないことが起きた時、切り戻しが楽。
納期の都合上、優先度の低いものを先送りするときなどにも、分かれて管理できていると作業が楽。
さいごに
私自身、社内の複数の背景が異なるプロダクトでレビューする経験を経て、コードを大体読めれば、業務ドメインの深い知識や、アプリの特性への知見が浅くとも、そこでレビューする意味があると実感しています。
レビューの目的は共通知を得ることだとか教育だとか世には色んな意見がありますが、私は仕事上でのレビュワーとしての仕事は、お客様・自分たちの未来が少しでも問題の少ない世界にするためのガーディアン活動かなと思っています。
これまで流した汗と涙から厳選したポイントたちです。これらだけでも、痛い目に遭う確率は結構減らせるんじゃないかと思います。
巨大な既存コードに手を入れる方、レビュワーとして修正にGOサインを出す立場になったばかり方、二の轍を踏む人が少しでも減らんことを祈っています。
メリークリスマス！