僕がフロントエンドのコードレビューをする時に意識していること
この記事は YAMAP エンジニア Advent Calendar 2021 の10日目の記事です。
はじめに
コードレビュー難しいですよね。
今年で6年目になりますが未だにコードレビューが出来ると胸を張って言うことができません。
予定よりも時間をかけてしまったり、そこまで指摘しなくても良かったなーという点を細かくレビューしてしまったり、逆に指摘すべき箇所を見逃してしまったり問題点を挙げればキリがありません。
コードレビューのことを考えるにあたり、自分が普段どんな感じでレビューしているか意識したことをがないなと思い、一度自分がレビュー時に意識していることを整理してみました。
仕様の把握
コードレビューを始める時に自分が最初にやることは、修正内容の仕様の把握です。ここでいう仕様とは、「どういう場合にどういう状態になるか」を指しています。
コードレビューとは修正されたコードに問題がないかを客観的な目線でチェックする作業です。
このとき、何が正しいか の基準が無いと問題が無いことを判断することはできないため、最初に何が正しいのか 知るためにも対象のPRでの仕様を理解することを意識しています。
概要欄を読み込む
チームでレビューをしていると、PRの概要欄には修正の目的などが記載されています。
まずは概要欄を読むことでざっくりと全体を把握できる事が多いので、最初に概要欄をしっかりと読むようにしています。（しっかり書かれている前提ですが）
ブラウザで動作確認
修正内容を理解するために、ソースコードを読む前にブラウザで実際に動作確認をします。
先に実際に動かして確認をした方が理解を深めやすく、何をやっているかをイメージしながらコードリーディングができるので、コードレビューがやりやすくなります。
最初の動作確認で分からない点や気になった箇所があれば、そこだけピンポイントでコードを読んで仕様を理解するようにしています。この時にバグが見つかることも多々あります。
APIリクエストの仕様について
APIリクエストをしている場合はAPIレスポンスのデータ構造も意識的に確認するようにしています。
フロントエンドはAPIのレスポンスデータを画面上に表示する事が主な役割なので、表示するデータの仕様を把握しないと正しくデータを扱えているかの判断が難しいためです。
外部APIのレスポンスでデータの意味が分からない場合は、念の為API仕様書でレスポンスの仕様を把握してレビューができると、予想外のバグが見つかったりします。
レビューの優先度
レビューをする時は自分の中で項目を分けて優先度を付けてレビューをしています。
- バグ・デグレが発生しないか
- セキュリティは問題ないか
- 変更は容易か（設計）
- コードは読みやすいか
レビュー項目を分けて優先度を付けておくと、PRの内容に応じてレビューにかける時間配分をコントロールできるようになります。
- コードレビューをする時間が少ない場合は最低限 2 まではしっかり確認して 3,4 にかける時間を短くする。
- 今後も変更が多い重要な機能は、多少を時間をかけてでも全ての項目について細かく確認する。
重点的に見る箇所
- サービスとして重要な機能やお金が絡む部分
- XSSなどのセキュリティバグが発生しやすい部分
- クエリパラメーターを内部で参照する場合など
- 多くの場所から参照されているファイル
- 共通ライブラリやコンポーネント
バグ・デグレが発生しないか
- 意地悪な気持ちになって特殊な操作をしてみる
- PC,スマホの両方のviewportでデザイン崩れが発生しないか
- CSS, JS はサポートしているブラウザで利用可能か
- ここら辺はESLintで自動テストをかけて、ある程度チェックを省略することもできます
- 条件分岐の条件は適切か
- 修正箇所を呼んでいる他の部分に影響がないか
バグ・デグレが発生やすい箇所
次の箇所は経験的にバグ・デグレが発生しやすいので注意深くレビューをすることが多いです。
- コードの可読性が低い部分
- コードから実装者の意図が読み取れない箇所
- 裏側に複雑な仕様が隠れていることがあるので、デグレが発生しやすい
- データ構造が複雑な箇所
- 多くのことをやり過ぎている可能性が高く処理が複雑になっている可能性が高い
- 条件分岐が複雑な箇所
- 多くのことをやり過ぎている可能性が高く処理が複雑になっている可能性が高い
- 多くの箇所から参照されている部分
- 呼び出し元に応じた処理が1箇所に書かれて、処理が複雑になりやすい
- パッと見で特殊なことをやっている箇所
- 読んでいて普通だったらこんな実装しないなと感じる箇所は裏側に複雑な仕様が隠れていることがあるのでバグが発生しやすい
- なんかモヤモヤとした不安感を感じる箇所
- モヤモヤの正体は経験則による直感であることが多く、過去に似た箇所でバグに遭遇したことがあったりするので、その直感は当たることが多い
セキュリティに問題はないか
- URLパラメーターなどユーザーが入力可能な値を参照している場合は、XSSなどに問題がないか
- 権限などで機能制限をしている場合は、ローカルプロキシなどでレスポンスを改竄して悪さできないか
- レスポンスを改竄して制限をすり抜けられる場合は、それをやられた場合のサービスのリスクを考える
変更は容易か（設計）
- 発生する可能性のある仕様変更を想像して、その仕様変更に対して容易に対応可能か
- 仕様変更があった場合に複数箇所を修正する必要があると1箇所だけ修正を忘れてデグレが発生しやすい
- 修正時の影響範囲が多いとデグレが発生しやすい
- 一つのファイルが責務を持ち過ぎていないか
- 1箇所で多くのことをしていると処理が複雑になりデグレが発生しやすい
- 修正時の影響範囲も把握がしづらくなる
- 修正時に考慮することが増えるので機能改修に時間がかかる
コードの可読性
- 1か月後の自分が同じコードを読んで理解できそうか
- 無駄に複雑に書かれていないか
- 変数名は誤解を生むような名前になっていないか
- 自分が実装する場合にどう実装するかをイメージする
レビューコメント
- 可読性や設計を指摘する場合は、極力ダメな理由と改善案（サンプルコード）を一緒に提示する
- 一方的にダメと言われるとレビュイーも困りますし、気分が悪くなっちゃいます。
- ラベルをつける
- レビュイーがコメントに対して優先度をつけて対応ができるので効率が上がります
- FIX(バグ修正）, REFACTOR、COMMENT など
- レビューコメントにラベルをつけるだけで開発効率があがって幸せになれそうな話 - Qiita
- 絵文字やAAを使う
- コードレビューは書いたコードに対して何かしらの指摘が発生する作業なので、ギスギスしてしまう可能性があります。
- 少しでもギスギスしないように絵文字やAAを使って雰囲気が柔らかくなるようにしています。
- 良い点があれば Good なリアクションをする
- 褒められると嬉しい👍
- 人を否定しない
- こんなコードを書くなんて全然ダメだ！などと個人に対しての言及はダメ。ゼッタイ。
- 自分が言われたら辛い🥲
- 宗教戦争になり得ると思った指摘はしない
- ただ、宗教戦争になるかどうかの判断はかなり難しい
- 書き方の問題であれば、コーディング規約を決めてLinterでチェックできると良い
- 宗教戦争になりそうだと思ったら、「好みの問題かもしれないですが...」などと前置きをおいてお茶を濁す
- COMMENT ラベル等でお気持ちの表明だけをする
コードレビューを楽しむ🎉
自分はコードレビューをゲーム感覚で実施することを心がけています。バグを見つけらたらレビュアーの勝ち！バグが無ければレビュイーの勝ち！という具合です。目標を持ってコードレビューをするとレビューをしていて楽しいですし、バグを見つけることに全力になります。
※ 勝ち負けは自分の中だけに留めておいてください。
参考資料
丁寧にまとまっているので、併せて読んでみてください。