コードレビューにおいて気をつけること

Last updated at 2024-12-12Posted at 2024-12-11

こんにちは。
レバウェル開発部アドベントカレンダー 12日目担当です。

入社してもうすぐ一年が経つのですが、コードレビューで何を見ればいいのか、最近までよく分からず、「なんとなく」でやってしまっていました。

このままではいかん！ということで、今回はコードレビューについて色々と調べてまとめてみました。（参考文献は、記事の最後にまとめています）

大きく「心構え」と「引き出し」の二つに分けて解説します。

心構え編

まずはコードレビューに臨む上での心構え、考え方についていくつかご紹介します。

よくないコードを放置しない
→ 問題のあるコードを見つけたら、なるべく早く修正しましょう。時間がないときはメモやタスクに残し、後で対応できるようにします。放置は負債につながります。
自分や相手の立場を忘れる
→ 「この人は経験豊富だし、自分より技術力高いし、この人の書いたコードなら、まあいいでしょう！」とスルーするのはNGです。相手の経験や技術力に関係なく、コードそのものを客観的に見ましょう。
しょうもない疑問や指摘を恐れない
→「こんなこと、みんな知ってるかも」と思っても、遠慮せず質問や指摘をしましょう。たとえ知っていたとしても、認識を揃える機会になります。
完璧を追求しすぎない
→ コードレビューに時間をかけすぎないことも大切です。「すべてのバグを見逃さない」というのは現実的でないので、レビューの深掘りは緊急度などに応じてメリハリをつけましょう。
自動化できる部分は自動化してあげる
→ 静的解析ツールやLintツールを活用すると負担を減らせます。本質的な部分のチェックに集中できることが理想です。

引き出し編

コードレビューは、「より良いコード」にするための作業です。

では、「より良いコード」とは具体的に何でしょうか？
いくつかの指標をもとに、コードを改善する方法を考えていきます。

凝集度

まず、凝集度という観点について見ていきます。

凝集度とは、単一責務の原則と近い考え方で、モジュールのまとまり具合に関する指標です。

同じ役割のものが一箇所にまとまり、関係のないものは分離されている状態を「凝集度が高い」と言います。この状態は良い設計の目安になります。

凝集度が高いと、以下のメリットがあります。

メンテナンス性の向上
→ 機能とモジュールが1対1の関係になっていると、コードは理解しやすく、保守もしやすくなります。
影響範囲の縮小
→ 修正が他の部分に影響を与えるリスクが減ります。

凝集度にはいくつかのレベルがあり、すべてを機能的凝集にするのは難しいですが、意識するだけでコードの品質は向上すると思います！

偶発的凝集

最悪な状態です。以下の特徴を持っています。

無作為に集められたものがモジュールになっている
それぞれの処理に関連性がない
共通の目的がなく、偶然まとまっている

例

function sampleProcess() {
   $user = getUser(); //データベースからユーザーを取得する

   $formatDate = date("Y-m-d H:i:s"); //日付をフォーマットする

   $randomNumber = rand(1, 100); //ランダムな数値を生成する
}

この関数は、それぞれの処理に関連性がなく、単一の目的を持っていません。

改善方法としては、以下のように処理を目的ごとに分割すると良いです。

function getUserFromDB() {
    return getUser(); // データベースからユーザーを取得する
}

function formatCurrentDate() {
    return date("Y-m-d H:i:s"); // 日付をフォーマットする
}

function generateRandomNumber() {
    return rand(1, 100); // ランダムな数値を生成する
}

凝集度が向上し、コードの可読性と保守性が高まります！

論理的凝集

論理的凝集は、次の特徴を持っているものです。

外部からの入力（フラグやパラメータ）によって処理が決まる状態
一つの関数に複数の処理が詰め込まれている

例

function sampleProcess($flag, $data) {
    if ($flag === 'read') {
        //データを読み込む処理
    } else if ($flag === 'save') {
        //データを保存する処理
    } else if ($flag === 'delete') {
        //データを削除する処理
    } else {
        //その他の処理
    }
}

//使用例
sampleProcess('save', 'sample data');

こちらも関数が複数の責務を持ってしまい、凝集度が低い状態です。

フラグごとに処理を独立させ、個別の関数に分けると凝集度が高まります。

一時的凝集

一時的凝集は、特定のタイミングで一度に実行される処理が一つの関数内にまとめられている状態です。

例のように、「同じタイミングで実行される」という共通点はあるものの、処理内容自体には関連性がありません。

また、実行順序に意味がなく、処理を入れ替えても問題ないです。

例


function initialize() {
   // 〜 セッションを開始する処理 〜

   // 〜 データベースと接続する処理 〜
   
   // 〜 セッションを開始する処理 〜
}

//使用例
initialize();

こちらも同じように、処理をそれぞれ独立した関数に分け、必要なタイミングで個別に呼び出すようする、もしくは、責務ごとにモジュールを分けることでコードの可読性、拡張性が向上します。

手順的凝集

手順的凝集は、一連の処理が特定の手順に従って実行される状態です。
それぞれの処理は依存関係を持ち、特定の順序で実行しなければなりません。

例

function processFormSubmission() {
    // 〜 入力データのバリデーション 〜
 
    // 〜 データベースへの保存 〜
    
    // 〜 確認メールの送信 〜
 }
 
 //使用例
 processFormSubmission($formData);

手順的凝集は偶発的凝集や論理的凝集に比べて、凝集度が比較的高いですが、処理が1つの関数にまとまりすぎるとメンテナンス性が低下します。

通信的凝集

通信的凝集は、同じデータを扱う処理が一つの関数内にまとまっている状態です。

以下の例の処理は共通のデータを共有しているため、関連性が高いように見えますが、役割が分散している場合、凝集度が低いとも言えます。

function processUserData($userId) {
    $user = getUserById($userId); // ユーザー情報を取得
    logUserActivity($user); // ユーザーのアクティビティをログに記録
    sendWelcomeMessage($user); // ユーザーにウェルカムメッセージを送信
}

// 使用例
processUserData(123);

逐次的凝集

逐次的凝集は、ある処理の出力が次の処理の入力になる場合、見られる状態です。
処理が順次的に進むため、一連の流れを理解しやすいメリットがあります。ただし、各処理が強く結びつきすぎている場合、変更に弱くなることもあります。

例

function processOrder($orderId) {
    $order = fetchOrderDetails($orderId); // 注文詳細を取得
    $validatedOrder = validateOrder($order); // 注文を検証
    $receipt = generateReceipt($validatedOrder); // 領収書を生成
    sendReceipt($receipt); // 領収書を送信
}

// 使用例
processOrder(456);

機能的凝集

一番理想的な形です。
単一のタスクを実現するような関数です。

 //2点間の距離を計算
 function calculateDistance($point1, $point2) {
    $xDiff = $point2->x - $point1->x;
    $yDiff = $point2->y - $point1->y;
 
    $distance = sqrt(pow($xDiff, 2) + pow($yDiff, 2));
    return $distance;
 }

すべての関数をこの形にすることは難しいかもしれませんが、目指すべき理想形として心がけましょう！

品質特性

次に、可読性、保守性、効率性、信頼性、拡張性の観点でそれぞれ確認すべき点をまとめました！

可読性

意図が分かる命名か
→ 変数や関数の名前がその役割や目的を的確に表しているか。曖昧な命名でなく、具体的な名前を付けましょう。
インデントがずれてないか
→ コードの階層構造が正確に反映され、見た目が整っているか。
ネストが深すぎないか
→ ネストが深いとコードの流れが追いにくくなります。早期リターンやガード節を使えないか検討しましょう。
ロジックをシンプルにできないか
→ 複雑なロジックを簡略化することで、理解や保守しやすくなります。不要な分岐や処理は減らしましょう。
コメントは適切か
→ 必要な場所にのみコメントを残し、コードの目的や意図を補足するようにします。冗長なコメント、コードを読めば分かるコメントになっていないか。
単一責任になっているか
→ クラスや関数が一つの明確な目的を持っているか。

保守性

マジックナンバーが使われてないか
→ 数値や文字列（マジックナンバー）を直接コードに記述すると意図がわかりにくくなります。定数を使うか設定ファイルに定義しましょう。
重複したロジックがないか
→ 共通メソッドや関数にまとめることで変更箇所を最小限に抑えられます。
インターフェースや抽象クラスを活用できないか
→ 変更時の影響範囲を抑えられます。
ユニットテストがあるか
→ 変更後の動作確認が容易になり、不具合をいち早く発見できます。
スコープが適切か
→ 必要最小限のスコープにすることで、他コードへの影響を減らし、意図しないバグの発生を防げます。
public privateなどアクセス修飾子が正しいか
→ クラスやプロパティ、メソッドに適切なアクセス修飾子を付与することで、外部からの不必要なアクセスを防ぎ、意図しない不具合を防止できます。

効率性

インデックスが貼られているか
→ データベースのインデックスを適切に設定することで、検索や取得の速度を向上させることができます。主キーやよく検索されるカラムには設定しましょう。
不要なデータ取得がされていないか
→ 必要以上に多くのデータを取得していないか。必要な列だけを取得するようにクエリを最適化できないか。（例：SELECT * の使用を避ける）
キャッシュできないか
→ 頻繁に使われるデータや計算結果はキャッシュすることで、リソース消費や処理時間を削減できます。ファイルキャッシュ、メモリキャッシュ（Redisなど）を活用できないか。
不要なDBへのアクセスがないか
→ 同じデータを繰り返し取得していないか。一度だけ取得して再利用するようにできないか。
非同期処理にできないか
→ 非同期処理を利用することで待機時間を短縮し、全体のパフォーマンスを向上させることができます。バックエンドでのキューやフロントエンドでの非同期API呼び出しが使えないか。
Eager Loadingの使用
→ リレーションを多用する場合、必要なリレーションデータを一括で取得することで、データベースアクセスの回数を減らし効率化できます。

信頼性

バリデーションに漏れがないか
→ 不正なデータや期待しない形式のデータが処理に渡らないようなっているか。
エラーハンドリングが適切か
→ エラーが発生した際に放置されず、適切なエラー処理を実装しているか。ユーザーに分かりやすいメッセージが提示されているか。
例外処理がちゃんとされているか
→ 例外を適切にキャッチしているか。例外が発生した場合でも最低限の動作が保証されるようになっているか。
適切にエラーログを吐くようになっているか
→ エラーや例外が発生した際に、ログとして記録されているか。詳細なログがあれば、問題発生時にスムーズに対応できます。
データベースの整合性が保たれていそうか
→ トランザクション管理や外部キー制約を活用し、更新や削除操作が中途半端に終了しないようになっているか。

拡張性

インターフェースを使用できないか
設定値の外部化
→ 設定値や環境依存の情報（例: APIキー、データベースの接続情報）はコード内にハードコーディングせず、環境変数や設定ファイルに外部化しましょう。
引数が多すぎないか
→ 引数が多すぎると、コードの理解が難しくなるだけでなく、再利用し辛くなるので、できるだけ少ない方が良いです。
単一責任になっているか
→ 単一責任になっていると、将来的に切り出しやすいです。
デザインパターンを使えないか
→ 拡張性を意識したデザインパターン（例えば、ストラテジーパターンやファクトリパターン）が使用されているか。将来的な拡張が簡単になります。

まとめ

以上になります。いかがだったでしょうか。まだまだ他にもあるかと思いますので、この先見つけたら追記していこうと思います。

ぜひ参考にしていただけると嬉しいです！

参考書籍

達人プログラマー: 熟達に向けたあなたの旅
 良いコード/悪いコードで学ぶ設計入門 ―保守しやすい成長し続けるコードの書き方
 リーダブルコード ―より良いコードを書くためのシンプルで実践的なテクニック

レバウェル開発部Advent Calendar 2024

Day 12

You get articles that match your needs
You can efficiently read back useful information
You can use dark theme

What you can do with signing up