コードレビューをなめるな、と強く言いたい
開発において一番大切だなーと思っているのがコードレビューだったりします。
コードレビューがしっかりしているところはきちんとしたプロジェクトだし、きちんとしていないところはダメなプロジェクトだと言い切っても良いくらいです。
過去様々なプロジェクトで働いてきましたが、多少の差はあれど、レビューが軽視されているなーと思うこともしばしばです。
なので今回は、自分がなぜレビューを重要視しているのか書いていこうと思います。
コーディング力が最も上がるフェーズである
コーディングを行っていると、いつの間にか視野が狭くなってしまっていることがしばしば起こります。
噛み砕いて言うなら『自分のコーディングは正しい』と思い込んでしまうわけです。
自身の書くコード 1 つ 1 つにエビデンスを持つことってすごく大切だと思うのですが、知識が古かったり間違っていたり、そんなこともしょっちゅう発生するわけです。
そんな独りよがりのコードを正してくれる唯一のフェーズがレビューだと思っていて。
レビューによって自分のコードに他の人の意見や考えが入ってきて、自分の考え方がアップデートされるわけです。
【コードを書く → レビューを受ける】の繰り返しで、少しずつ独りよがりのコーディングから離れていき、コードにエビデンスが追従する形が形成されていくわけで。
逆にコードレビューが行われないと、一生独りよがりのコードを書くことになっちゃうわけで、結構ゾッとします。
その人となりが見えてくる
コードレビューって、どちらかといえばコードレビューをする側のほうが難しいです。
言ってみれば先生と生徒の関係性に似ているわけで、先生のほうがやはり難しいですよね。
いかにしてわかりやすいレビューを行うべきか。
文字ベースでのコミュニケーションが発生する以上、表情などは見えないわけで。
日本語でのレビューとなると、敬語の問題なども発生するので、フランクさもこれまた難しいです。
自分の考え方をいかに誤解を与えず伝えるか、絵文字をたくさん使ったり、責め立てるような誤解を与えないような表現を使うことがとても大切なわけです。
で、コミュニケーションを重要視している人としていない人で、書き方ががらっと変わってくるのが面白いですよね。
責め立てるような表現をお互いに使い続けていると、プロジェクトに対するモチベーションは下がるし信頼関係は薄まるし、良いことなんてこれっぽっちもないと個人的には思っています。
責任が生まれる
コードレビューが通ったコードがマージされた後にエラーが発生した場合、レビューを行った人のチェックが甘かったという判断になります。
といっても、実際にはコードレビューくらいで全ての使用漏れやバグを見つけるのなんて到底無理ですし、個人的にはコードを書いた人も良くなかったよね、という判断になることが多いです。
つまり痛み分けと言うか、どっちもどっちだよね、という感じです。
とはいえ、コードを書く側は言わずもがな、レビューを行う側にも結構な責任が生まれることになります。
チーム全体で同じ責任を持つ状況が、コードレビューによって生まれるわけです。
個人を責め立てるようなことは決して起きてはいけないわけで、プロジェクト全体に少しの緊張感が生まれるのは良いことだよなーと。
プロジェクトの問題が見えてくる
コードレビューを行う際、ある一定のルールに則って行っているプロジェクトも多いと思います。
例えば自分がリードエンジニアを任されたプロジェクトでは、以下のルールをしくことが多いです。
- PR には必ず Issue を紐付けること（コーディングの初心者が多いプロジェクトに限る）
- コミットメッセージに close コメントを記述すること
- 1PR で 1000 行以上の修正を入れないこと
- Approve する場合は必ず「LGTM」と書くこと
- レビューは開発フローの中で最優先で行うこと
- 「レビューお願いします」といったことは書かないこと、気づいたらすぐにレビューを行うフローを取ること（Slack などで GitHub のログを流しメンションをつけるフローを取る）
- コミットはなるべく 1 本で終えること、コミット数が多くなる場合は Issue の粒度に問題がある
- 絵文字を多用すること
- 「must」「nits」「imo」のような接頭辞を使うことが好ましいが、強制はしない
- PR 上のやり取りで往復が発生しそうな場合は口頭で確認すること
- PR のメッセージにごちゃごちゃ書かない、PR の first commit のコミットメッセージをそのまま反映すること
などなど、大雑把にこんな感じです。
で、逆にレビューにルールがない現場では、様々な問題が発生します。
過去あったケースをいくつか書いてみると。
上司がレビューに追われていて、仕様の確認などがはばかられる
自分が正社員 2 年目くらいのときのプロジェクトです。
上司が常にコードレビューに追われていて、声をかけることすらもできませんでした。
おそらく、PR の粒度に問題があったように思います、1PR に対する修正量が多すぎたのかなぁと。
あと、10 人を超えるプロジェクトであったにも関わらず、有識者が上司 1 人というのもかなり問題でしたね。
かなり精度が求められるプロジェクトだったので、上司のレビューが必須なのはわからなくもないですが、もう少しなんとかならんのかと当時から思っていました。
PR がたまり続ける、Issue が Close されない
一番よくないケースですね、原因はたくさん考えられます。
- レビューを行う側がレビューを後回しにしている
- レビューを行う側の技術力が足りていない
- PR の粒度が大きすぎて、レビューに時間がかかっている
アジャイル開発だと、特に PR の粒度を小さくすることが大切で、引いては Issue を細かく切ることに繋がります。
PR や Issue の粒度って小さければ小さいほど得が大きいと思うんですけど、なかなか賛同してくれる人は少ないようで…。
ステージング環境や本番環境でエラーやバグが発生する
レビューが甘いケースで起きがちです、これも PR の粒度が粗いのが全ての原因ですね。
ローカルでの確認漏れによってよく起きるやつですが、修正範囲が大きくなればなるほどエラーやバグは出やすくなります。
フロントエンド開発で具体的にどの粒度で PR を上げてもらうかというと、
- AtomicDesign などを導入している場合、1 コンポーネントで 1Issue1PR（紐づくストーリーやテストも含む）
- 1 つの npm パッケージのインストールで 1Issue1PR
- Redux を使用している場合は 1Action に紐づく諸々で 1Issue1PR
といった感じのフローを取ります、なので Issue も PR もかなりの数になりますね。
その分レビューも大量に行うのですが、1 レビューで割かれる時間自体はかなり短いです。
むしろ早い段階でメンバーの認識の相違を汲み取れるため、メリットは大きいと思います、コミットログも見やすいですしね。
自身、若い頃はとがったレビューを行ってしまい、先輩から苦言を呈されることも何度かありました。
そんな中で以下のドキュメントに出会ったのですが、目からウロコとはまさにこのこと、自分の考え方をガラッと変えてくれました。
コードレビューとは、受ける側も行う側も、相手のことを思ってコミュニケーションを行うことがとても大切です。
どうしても若いうちはとがってしまいがちだとは思います、自分も最初絵文字を使うことはすごく抵抗がありました。
とはいえ、レビューによって自分の人間性を正しい方向に修正できると思えば、意外とすんなりいけるのかなぁと。
自分の考え方に固執せず、柔軟に受け入れることがすごく大切だよなーと。