LGTMの意味がバラバラなんじゃ

最近レビューしてる時にこう思った
LGTMの意味合いって人によって違くない？？？

例えばこう

• 受け入れ条件が満たせていればlgtm
• 可読性や保守性が高ければlgtm
• テストが書かれていればlgtm

どれも間違ってはいないけど、それぞれが“違う基準”でLGTMをつけていることになる

それだと何が困る？

• これだとコードの品質にムラが出る
  • 将来の自分たちの苦しめることになる
  • 実際そうなってる
• 人によってレビューで意見するところが違う
  • 可読性について意見してるの俺だけじゃね？とか
  • 何をどこまで意見していいのか迷う

まずはコードレビューの目的を明確にしよう

「プロジェクト全体のコード品質を守る文化を育てること」
ここでいうコード品質とは次の３つ

• 可読性が高い
• 保守性が高い
• 仕様を満たしている

仕様を満たしているは大前提として、自分たちが開発しやすいコードを作れるように意識したい

コードレビューの観点

目的を達成するためのおおまかな観点を整理する

1. 仕様を満たしているか
   1. テストで仕様が担保されているか
   2. バグを埋め込んでいないか
2. チームメンバー全員が理解しやすいコードか
   1. 可読性
   2. 保守性

人によって異なるコードの読みやすさ

人によってどういうコードが読みやすいのか、判断を揃えるのは難しい
とりあえず一つの基準を設ける

• 「半年後の自分」「新メンバー」が理解できることを基準に

あとは、やりながらチームで調整していく

コーディングガイド

チームの設計方針みたいなものを作ってるので、それを守ることで読みやすさをある程度担保していこう

設計の一部

• データ取得ロジック
  • QueryObject
  • FinderObject
• 判定ロジック
  • Rule
  • Policy

作ったもの

以上を踏まえてコードレビューの時に使うチェックリスト作ってみた

1. 仕様通りに動作するか（IPAの機能点検）
   • 受け入れ条件を満たした実装になっているか？
     • データのscopeに問題はないか？
     • ユーザ認証は適切か？
2. テストの有無と品質
   • テストが存在し、対象ロジックを網羅しているか？
   • 正常系・異常系・境界ケースをカバーしているか？
   • テスト名は適切か
3. 可読性・保守性
   • 大まかな処理の流れが一目で理解しやすい構造になっているか？
   • 命名は一目で意味を理解できるものになっているか？
   • メソッドやクラスの責務が適切に分離されているか？
   • チームのコーディングガイドに則っているか
4. API設計
   • パス設計ができるだけRESTfulを遵守しているか
     • あくまでも直感的に理解、使用できるかがポイント
     • 遵守しない方が理解しやすいのであれば無理にRESTfulにする必要はない
   • HTTPメソッドが適切に選ばれているか？（GET/POST/PATCH/DELETEなど）
   • ステータスコードが正しく使われているか？（200/201/204/400/404/422/500など）

常に目に入るようにする

せっかく作ったチェックリストが結局全然使われなくて風化していくと困るので、
MRを作成したときに自動的にoverviewに含まれるようにした
これでレビュイーもレビュアーもMR見た時に自然と目に入るようになる

コードレビューでしないことも決める

観点とは異なるけど、レビューをより良いものにしようということで
レビューでやらないことも決めた

• [nits]なことは書かない
  • 修正が必要ないことは書かない
• 深追い調査はしない
  • 例：パフォーマンスの懸念があるなら、実装者に確認を依頼する

つまり、

• 時間をかけない
• 余計なことは言わない

導入した結果

まだ導入して2週間ほどしか経ってないが、今のところ導入して良かったっぽい
メンバーからの意見↓

• レビュー時に見るべきポイントが明確化し、「どこを見ればいいか分からない」が解消された
• 全員が同じ観点・基準でレビューできるようになり、レビュアー間の質の差が減少した
• 実装者側もレビュー観点を把握できるため、実装漏れや考慮漏れが減った
• チェック項目の関心がチーム内で高まった
• そもそもこういうことをチームで改善しようとすることに意味があるよね