引き継いだプロダクトが「コードレビュー？なにそれおいしいの？」からのコード無法地帯状態で、
改修は難しいし、改修のたびに重大バグとか起こしていたので、
恨まれてもいいからレビュー改革を断行したら、
逆にメンバーが成長して、しかも感謝されたのでよかったら参考にしてください。
※注意！この記事にはExcelが含まれます。
※Excelにアレルギー症状がある方は読むのを中止されることをお勧めします。
※なお、Excel設計書をWikiに置き換えた話はこちら
3つ施策
- 関数単位でOK,NGを記録する
- 四回に分けてレビューする
- リジェクト基準を作る
解説
1. 関数単位でOK,NGを記録する
| ○/× | 効果/副作用 |
|---|---|
| ◎ | 指摘密度 大幅増 |
| ○ | メンバーの修正負荷 低 |
| × | レビュワーの負荷 大幅増 |
ソースコードレビューで非熟練者がよくやりがちなのが、
レビューしているようで「実は眺めていただけだった」「ソースを追いかけていただけだった」という失敗である。
ソースコードレビューとは動作させるだけではわかりにくい欠陥を摘出する行為であり、
品質向上に資する指摘をしないと意味がない。
とは言っても、一度に１００行、２００行のソースコードをレビューしようとすると、
どうしても細かいところのチェックがおろそかになってしまう。
だったら細かいところに意識を集中させればいいと考え、試行錯誤してたどり着いたのが、
関数単位でOK、NGを記録すること。
こうすることで、関数ごとに細かくチェックできるし、関数単位でチェック漏れがなくせる
上記の図は、最初期のレビュー記録表のイメージ図だが、
これを導入することで以下の効果/副作用が発生した
- これまで挙がらなかった指摘が大幅に増えた
- 指摘が増えたけど、メンバーにとってどこをどう直せばいいかまとまっているので、修正負荷は減った
- 記録が残るので、その指摘に意味があるのかという議論の土台になり、
徐々に指摘内容が改善された。 - また、上記の議論自体がメンバーの成長材料となった
- ただし、レビュワーのレビュー業務時間は大幅に伸びてしまった
2. 四回に分けてレビューする
| ○/× | 効果/副作用 |
|---|---|
| ◎ | 指摘価値 向上 |
| ◎ | 試験工程でのバグ率、負荷 大幅減 |
| × | レビュワー 負荷 増 |
ある程度経験を積んだレビュワーがやりがちな失敗は、
指摘しやすいコーディング規約違反だけ指摘している
というもの。
コードレビューで指摘するべき欠陥とは、必ずしも規約違反だけではなく、
仕様考慮もれや機能的なバグ、非機能的なセキュリティやパフォーマンス上の問題点も含まれる。
一つ関数に対して複数の視点でソースチェックをしないといけないが、
人間は同時に複数のことは考えられない。
そこでどうすればいいかと情報をあさっていたところ、
われらがIPAがセキュアプログラミング講座というWEBページで、
四回に分けてレビューすることを提唱していた。
１回目はどこに何があるか、
２回目は可読性が確保されているか、規約にのっとっているか
３回目は機能性
４回目はセキュリティ
といった具合である。
IPAの講座では４回目はセキュリティに限定しているが、
担当していたプロダクトは、非機能面はセキュリティはもちろんパフォーマンスも重要だったので、
上記の改善版レビュー記録表では、４回目はセキュリティ・パフォーマンスとして、
このIPA提唱方式に乗っかてみることにしたら、
- 意味のある指摘がさらに増えた
- 試験工程でのバグ率が極端に下がり、したがって修正・再レビュー・再テストという工数が大幅に削減された
- さらには、指摘の過程で、どういうコードが脆弱性を生むのか、
あるいはどういうソースがパフォーマンスを悪化させるのかを、
逐一叩き込まれたメンバーは、あっという間に高品質なソースを生み出せるようになった - ただしやっぱりレビュワーの負荷は上がった
3. リジェクト基準を作る
| ○/× | 効果/副作用 |
|---|---|
| ○ | リジェクトに対するメンバーの納得感 |
| ○ | レビュワー 負荷軽減 |
熟練レビュワーでもやりがちな失敗は、
その日の気分・体調によって、「読みやすい・読みにくい」の評価が変わってしまい、
レビューイがヘイトをため込んでしまうやつである。
機能性の「バグってる・バグってない」や「非機能性の例えば脆弱性がある・ない」は、
YESかNOか判定可能であるが、可読性や保守性とは程度であり、
基準が示されなければ、判定結果は「気分」と言われても仕方なく、実際気分で変わってしまう。
私は指摘内容が「気分」と言われたくなかったのと、
「だれだこのゴミソース書いたやつ⇒すまん俺だった」の常習犯だった自分自身を一番信用してなかったので、
以下の基準等を明示することにした。（ほかにもある）
- コード行数30行以内。20行以下推奨。
- 極力コンパクトにしておかないと全体把握にパワーを割かれ、
読み間違え、バグのリスクが高まるから- 30行以内なのは、スクロールが必要かどうかを基準とする
- なお、SQL操作関数など、どうあがいても30行に収まらないものもあるので絶対ではない
- 行数が多いのは、大体なんでもかんでも一つの関数にまとめすぎなので、関数を分解すること
- 循環的複雑度20以内。10以内推奨
- 循環的複雑度は、wikipedia参照
- 計算はIDE自体に用意されているか、Lint等プラグインを導入することでPCに算出させることができる
- 循環的複雑度は、必ずカバレッジC1 <= 循環的複雑度 <= カバレッジC2 が成り立ち、
複雑度の数値が高ければ、それだけテストが必要になり、テスト工数は増えるし、バグリスクは高くなるしで、
リリースするのも大変だし、そのあと修正するのも大変- できれば10以内にしてほしいが、いったん定義に従い20を超えたらリジェクトする
- これは稀な例外を除いてほぼ絶対である
- 複雑度が上がるのは、大体（ｒｙ、関数を分解すること
- ネストは２段まで
- IFなどの条件やループなどによるネストは2段までとし、3段以上はリジェクトする
- 単純IFでも2段で4パターン考慮が必要で、3段になると8パターン考慮が必要
- 人間が苦も無く同時の考慮できるのは、せいぜい３～５パターンなので、2段に抑えること
- 闇深ネストが発生したら、とにかくガード節でネスト解除できないか試すこと
- それでも3段ネストが残るなら、関数分割を試行すること
- 関数名、変数名において、名詞を除いて中学校3年間で習う英単語以外使ってはならない
- みんなが読める必要がある。「みんなが読める」定義として「義務教育」を援用する
- なお、中学校3年間で習う英単語一覧は別途用意する（社内で業務として作ったので公開不可）
基準を明示すると、そもそも違反したコードがレビューに回ってくることが劇的に減り、レビュワーの負荷が下がる
さらにいうと、指摘⇒修正⇒再レビューというやり取りが減るので、メンバーの負荷も下がる
万が一違反ソースがレビューに回ってきても、納得もへったくれもないので、指摘でメンバー内にヘイトがたまらない
施策の結果
- コミットスピード、製造完了スピードは下がったが、テストフェーズでの不具合率が著しく下がり、
したがってメンバーの大半が大嫌いなテストフェーズの工数が著しく下がった - 改革直後からプロジェクト全体ので生産性が向上した
- またすべてのメンバーが1年もしないうちにメキメキと良いソースが書けるようになった
- メンバーもそれまでのエンジニア経験と、改革後の成長スピードの圧倒的な違いを実感しており、
「もっと早くからこういう現場で仕事したかった」と恨めしそうに言われた - 余所で「使えない」と判断されたメンバーが、このチームに来たら決済連携を含む難易度の高い実装ができるようになった
- レビュワーの負荷は改革直後から猛烈に上がったが、メンバーが大きく成長し業務を巻き取ってくれるようになったので、
一年もしないうちに、改革前よりちょっと忙しい程度に落ち着いた
考察
- コードレビューは品質向上のみならず、メンバーの成長にも有益である
- これはコードレビューがフィードバックとしての性格も有しているからである
- 成長のためにはフィードバックは質よりも量
- cf. 拙記事 新卒エンジニアの育成に、"多頻度マルチフィードバック"という概念を導入してみた結果。
- 緻密なレビューがフィードバックの量を生み、成長をブーストさせた説
- ３つの施策を以下のように抽象化することで、
コードレビューに限らず、すべてのレビューが改善できる（というかできた）- 関数単位 ⇒ レビュー対象の細分化による指摘詳細化・緻密化
- 4回 ⇒ 観点の限定による検知率の向上と、別観点での複数回実施による網羅性確保
- リジェクト基準 ⇒ 客観基準の事前明示による自律的な改善
雑感
- この施策でレビューしようとしたら、レビュワーとレビュイー比率は、１：３までにしないと、レビュワーつらい
- 実際、施策は3つどころの話じゃないくらい試したけど、大きな効果があったのはこの3つだった
- ちなみにこの話、7年くらい前の話なので、当社のお客様におかれましては、
現在はさらに改善されているのでご安心ください。 - 緻密なレビュー、頻繁なフィードバックは成長に大きく貢献するよねという記事を書いていてなんですが、
今では、レビューよりもアドバイザリじゃね？というあらたなパラダイムを信奉しています- いずれこのこの話も書きたい