⛳
【実践編】初心者エンジニアのためのコードレビュー入門

2024/12/16に公開
前回からの続きです

https://zenn.dev/sato_frontend/articles/62dd45cfab3b81
今回はコードレビューの流れやレビュー時に気をつけていることを紹介します。

 コードレビューの流れ僕はコードレビューをする時は下記の手順で確認するようにしています。

 1. PRのdescriptionを確認このPRではどんな変更がされているのかをしっかりと把握するようにしています。

僕はあまり容量が良くないので1-2回ざっくりと読みコードを確認しつつ中身を理解するようにしています。

 2. issueの確認PRの作業内容がissueと合っているかを確認します。
余談: issue範囲外のことはしない僕がチーム開発に慣れていない時は、issueの範囲外のこともやってしまっていました。

例えばissueとは関係ないコードでバグになりうる記述が合ったりタイポがあった時に良かれと思ってそも部分も直していました。

自分がレビュワーになって気づいたのですが、issueの範囲外の箇所でコードの変更があると余計なレビューコストがかかります。

そのコードが変更されたことで予期せぬバグが発生しないかなどの確認をしないと行けないからです。

そのため、もしissue対応中にissue範囲外で気になるコード等を発見したらそれ用にissueを作成するようにしています。

 3. 動作確認をする人によってはコードを見ただけでどのような動作をするか頭の中でイメージできるのかもしれませんが、自分はまだその域に達していないのでissueの内容を解決できているかを実際にブラウザで確認するようにしています。

もしここを確認せずにapprove(レビューが問題ないと判断し親ブランチへのマージを許可すること)しバグが発生した場合、自分に責任を感じるからです。

レビューを依頼されたら実装者と同じくらい責任を持ってレビューすることが重要だと思っています。

 4. コードを確認するここから実際にコードのレビューをしていきます。

 ケアレスミスのレビューレビュー中に下記のコードがあったとします。
type personalInfo = {
  name: string
  place: string
  hobby: string
}

const introdusePeople = (person: personalInfo): string => {
  return `私の名前は${person.name}です。${person.place}出身で、趣味は${person.hobby}です。`
}

const personInfo: personalInfo = {
  name: '太郎',
  place: '東京',
  hobby: '読書',
}

僕がレビュワーだったら下記を指摘すると思います。
型名の命名規則

TypeScriptでは、型名やインターフェース名は頭文字を大文字(PascalCase)にするのが一般的なので
type PersonalInfo = {
  name: string;
  place: string;
  hobby: string;
};
が適切です。
関数名のタイポ

関数名 introdusePeople にはタイポがあります。「紹介する」という意味の英単語は introduce なので、修正すべきです。
const introducePeople = (person: PersonalInfo): string => { ... };
関数名のリネーム

この関数は「1人の自己紹介文を生成する」ものなので、introducePeople（複数形）よりも、introducePerson（単数形）が適切です。
const introducePerson = (person: PersonalInfo): string => { ... };
できればこれらのミスは実装者が事前に気づいて修正できるのがベストですが、タイポや命名規則という部分は、適切でなくても問題なく動いてしまうので意外と実装者は気づかなかったりします。

実装者が見逃してしまったタイポ等を指摘できるように最初にざっと全体を見るようにしています。

 実装面のレビュー
 破壊的メソッドを使っていないかの確認Javascriptにおける配列の「破壊的メソッド」と「非破壊的メソッド」についてご存知でしょうか？

破壊的メソッドは、元の配列そのものを変更するメソッドで

非破壊的メソッドは、元の配列を変更せずに新しい配列を返すメソッドです。
Reactのプロジェクトなどでは一般的に破壊的メソッドの使用は避けるべきと言われます。

その理由として、破壊的メソッドは実行後に元の配列の内容が変わってしまうのでReactの状態管理で使うとバグを引き起こす原因になりやすい事などが挙げられます。

(状態が予期せず変更され再レンダリングが正しく動作しないなど、、、)

ローカル環境では破壊的メソッドを使用してもエラーは出ないのに、テスト環境や本番環境でエラーが出るケースなどが非常に厄介です。

プロジェクトで遭遇した時は解決までにかなりの時間を要しました、、、。
実体験からも破壊的メソッドの使用は避けるべきだと思っています。
例えば
const [numbers, setNumbers] = useState([3, 1, 4, 1, 5, 9]);

const handleSort = () => {
  numbers.sort((a, b) => a - b); // 状態を直接変更（破壊的メソッド）
  setNumbers(numbers); // Reactが再レンダリングを検知できない場合がある
};
このようなコードでは状態が直接変更されるため、バグの原因になりうるので指摘できるように気をつけています。

修正案を出すとしたら
const [numbers, setNumbers] = useState([3, 1, 4, 1, 5, 9]);

const handleSort = () => {
  const sortedNumbers = [...numbers].sort((a, b) => a - b); // スプレッド構文でコピーしてからソート
  setNumbers(sortedNumbers); // コピーされた新しい配列をセット
};
か
const [numbers, setNumbers] = useState([3, 1, 4, 1, 5, 9]);

const handleSort = () => {
  numbers.toSorted((a, b) => a - b); // toSortedを使用（非破壊的メソッド）
  setNumbers(numbers);
};
に変更することを提案すると思います。
1番目はスプレッド構文を使用してnumbers配列をコピーしてからsortにかけています。

2番目はtoSortedメソッドを使用しています。このメソッドは ES2023 で導入された新しい非破壊的なソート方法です。

toSortedは元の配列を変更せず新しい配列を返します。このため、スプレッド構文を使わずに直感的なコードを書くことができ、Reactプロジェクトでも安全に使用できます。
タイポ等の明らかなミスとは違い、実装者が理由あってこのコードにしているかもしれないのであくまで提案として指摘するようにしています。

 コードの最適化レビューの中でもし余裕があればコードのパフォーマンスや保守性の向上につながる提案をすることもできます。

例えば、関数のメモ化を検討するのもその一つです。ReactではuseCallbackを使って関数をメモ化することで、不要な再レンダリングを防ぎアプリケーションのパフォーマンスを向上させる場合があります。

 レビューに慣れていない場合の心構えとはいえ、レビューに慣れていない場合や余裕がない場合には、基本的なチェックに集中するだけでも十分です。
今回紹介した以下のポイントに重点を置くだけでも他レビュアーに取って大変助かると思います。
タイポ修正: コード内の小さなミス（例: 関数名や変数名のタイポ）を見逃さず指摘する。
動作確認: 提案されたコードが正しく動作するか、実際にローカル環境で確認する。
慣れてきたら命名の一貫性やコードの簡潔さ、リファクタリング案の提案などより高度な観点を追加していければいいと思います。

 まとめ以上が僕がコードレビューの時に気をつけていることです。

僕自身まだエンジニアとしての知識が浅く、コードレビューではいつも緊張しています。
ただ、コードレビューは単なるフィードバックの場ではなく、先輩エンジニアや同僚のコードを通じて学べる貴重な機会でもあります。命名規則や実装の意図を深く考える経験が、自分のスキルアップにつながると実感しています。
コードレビューに慣れていないと「ミスを指摘しなければいけない」「知識不足が露呈するのでは」と不安になるかもしれません。

でも最初はタイポや基本的なチェックだけでも十分だと思います。相手のコードをしっかり読むこと自体が重要で、それだけでチームの一員として貢献しています。
また、レビューを通じて自分が疑問に思ったことを素直に質問することで、知識を補完できる良い機会にもなります。レビューは単なる一方通行の作業ではなく「教える」と同時に「学ぶ」場でもあることを意識すると、さらに有意義な時間になると思います。
コードレビューの流れ

1. PRのdescriptionを確認

2. issueの確認

3. 動作確認をする

4. コードを確認する

ケアレスミスのレビュー

実装面のレビュー

破壊的メソッドを使っていないかの確認

コードの最適化

レビューに慣れていない場合の心構え

まとめ

Discussion
