先週、指を結構怪我してしまって、今週は半休業中です。
先日、同僚のソースコードの修正をレビューして、大変言いにくかったのですが、そのPRは却下して、意味のある単位に分割して新しくPRを作るようにお願いしました。
話し合って、納得はしてもらったのですが、なんか伝えきれなかったなとの、もやもやが自分の中に残ったので。
その時思ったことが、
複雑な変更をして、バグが出るのは、何か根本的に間違っているのではないか？
複雑な修正や、大規模修正したら不具合がでても仕方ないみたいな雰囲気を感じることがあります。
前職が、絶対ミスをしてはいけない工場のオペレーションなどの仕事をしていて、Web業界に転職したので、余計にそう感じるのかもしれないです。
もちろん、どんなに注意しても人がすることなので不具合は出るんですが、不具合出るのって、不具合出るような仕組みから、不具合出るんじゃないか？と思ったりします。
ように見える
却下したPRの修正に関してですが、印象として、コードの修正もまずくないように見える、仕様も外してないように見える。
ただ、修正箇所が多くて、本当に正しい修正なのか確信を持てる自信がない。
これは、レビュアーの力量の問題もあると思う。（つまり自分の力量）
１つのPRに40ファイルの修正は、多いといえば多いが、そんなに無茶苦茶というわけでもないかもしれない。
「ように見える」の理由として、リファクタリング、機能追加、機能の修正がごちゃっと一つのPRになっている。
それぞれの変更は、それぞれ正しく見える、でも、「ように見える」から、全体として、これは正しいへの確信が持てない。
時間をかければ確認できなくもなさそうだけど、レビューの焦点がブレてしまって、「ように見える」を抜けきれない。
と、つらつら書いていて、ネット上で情報ないかとググったら、同じように思っている人は結構いた。
「巨大プルリク1件vs細かいプルリク100件」問題を考える（翻訳）｜TechRacho by BPS株式会社
粒度を小さくする
「小さく」というのは、限りなくゼロ行に近づけるということです。たった1行の変更のレビューを嫌がるレビュアーはいないでしょう。プルリクのサイズ（=コードの行数）に上限を設けることで、粒度を下げやすくする効果が著しく向上します。レビュアーが1行ずつ精査しやすくなるのはもちろんのこと、レビュー時間も大きく削減できます。コードを定期的にマージできるようになり、品質にも自信を持てるようになります。
作業のスコープを絞る
コードの行数を削減するための重要なコツは、プルリクのスコープ（=機能のセット）を絞り込み、解決するタスクを1つにする（または密接に関連する少数のタスクに絞り込む）ことです。スコープを絞ることで、レビュアーの認知機能にかけられる負荷を大きく軽減できます。1件のプルリクでいくつもの問題をいっぺんに解決しようと欲張ると、ある問題がどのコードと関連しているのかを整理する作業がつらくなります
「ついでのリファクタリング」はしないこと
開発者は、問題に気づくとその場でリファクタリングすることがよくあります。もちろんリファクタリングはよいことですが、別のプルリクで行うべきです。そうすることでリファクタリングを早めにマージできますし、関係のない機能リリースに修正が押し込められることもなくなります。「ついでの改善」は、元々のプルリクのリリースが遅れれば巻き添えで遅れてしまいますし、最悪まったくリリースされなければそのまま失われてしまうでしょう。
複雑なものを複雑なまま扱うことで見えなくる
どんなに複雑なものも、分解すれば単純な構造に分解できます。
当たり前なんですが、複雑なものを複雑なまま取り扱うと、複雑です。
複雑な修正をレビューしても、やっぱり複雑なので、見逃してしまう不具合も増えます。
複雑なものを分解して、意味のある最小単位に分けるのは確かに手間もかかるし、難しいことも多いです。特に元の設計が悪かったりすると尚更。
でも、単純なものはレビューするのも簡単で、当たり前ですが、不具合にも気がつきやすいです。
他を壊さない作業の境界を見つけるには、The Mikado Methodがおすすめです。
日本語に訳されていないのが残念ですが、今の時代DeepL様のおかげで余裕で読めます。
The Mikado Method (English Edition)
複雑な変更をして、バグが出るのは、何が根本的に間違っているのか？
何が根本的に間違っているのか？
「複雑な変更」という時点でアウトの可能性が高いということだと思います。
- 複雑なものを複雑なまま扱わないで、単純な形に分ける
- レビューを機能させるには、複雑かどうかの基準はレビュアーのレベルに合わせる必要がある
そんなところかな？
