はじめに
コードレビュー、してますか？
皆さんは普段、どんなコードレビューを行っているでしょうか。
私は結構、直観タイプで、具体的な手法というものがありませんでした。
環境構築を変更するためのレビューなら「ファイル全部対応できてるかなー」ぐらいの気持ちですし、
ロジックを構築するコードなら仕様書と見比べて「漏れ」が無いかを確認する程度のレビューを行っています。
時折、上記のようなコードレビューに関する記事を見ては、
「なるほどなー」「勉強になるなー」と感心するばかりで、
じゃあそれを実践できるかと言われると、
「うーん」となります。
そんなことを繰り返しているうちに、
一つ疑問に思うことが出てきました。
「レビューって8割経験則やない？」
大半の技術職はそうではあるんですが、
それってレビューをするフォーマットや型が無いから、
経験則によるレビューが滅茶苦茶多くなるのではないかと最近思い始めたんです。
まあ実際に経験した失敗とかは脳内に焼き付くものなので、
そういった経験があるか無いかでレビューの質って大幅に変わるとは思いますが、
一方で「汎用的に使えるようなレビュー観点」というのがあれば、
経験則が無くてもレビューの品質が一定レベルで保たれるように感じます。
ということで、今回はChatGPTを使って、コードレビューのフォーマットを構築していきたいと思います。
チェックリストを作成
まずは、ベースとなるコードレビューのチェックリストを作成します。
ChatGPTとの会話を通じて、以下のようなチェックリストを作成してもらいました。
## **事前準備**
- [ ] コードが期待通りに動作することをテストで確認したか？
- [ ] コードには適切なコメントが記載されていて、他の人が理解しやすいか？
- [ ] レビュワーは適切に選ばれているか（コードの専門性、経験などを考慮した上で）？
## **コードレビューの実施**
- [ ] コードを一行一行丁寧に読み、問題点や改善点を見つけたか？
- [ ] コードの理解を深めるための質問を投げかけたか？
## **レビュー結果の反映**
- [ ] フィードバックを開放的な姿勢で受け入れ、コードを改善したか？
## **コードチェック**
- [ ] コードの可読性は良好か（命名、コメントなど）？
- [ ] パフォーマンスについて適切に配慮されているか？
- [ ] セキュリティ対策はしっかりと行われているか？
- [ ] バグの可能性はないか（例外処理、ロジックなどをチェック）？
## **フィードバック**
- [ ] フィードバックは具体的で、問題点だけでなくその理由や解決策も提供されているか？
- [ ] フィードバックを感謝し、反省点を見つけて成長に繋げられたか？
## **コミュニケーション**
- [ ] コードレビューの過程で開かれた議論が行われたか？
- [ ] フィードバックを通じて互いの理解と尊重が深まったか？
## **コードレビューツール**
- [ ] コードレビューツールは効率的に使用されているか？
## **レビューの定期性と規模**
- [ ] レビューは定期的に行われているか？
- [ ] レビューは小規模な変更に対して行われているか？
## **フィードバックの質**
- [ ] フィードバックは具体的で建設的か？
- [ ] フィードバックには良い点も含まれていて、相手を励ます形になっているか？
## **コードの品質**
- [ ] 変数名、関数名は適切に命名されていて、その役割を理解できるか？
- [ ] 一つの関数やメソッドは一つの機能のみを担当しているか？（単一責任の原則）
- [ ] コードのリファクタリングが必要な箇所はないか？（重複コード、長すぎる関数、大きなクラスなど）
- [ ] オブジェクト指向の原則（カプセル化、継承、多態性）が適切に適用されているか？
- [ ] デザインパターンが適切に使用されているか？
## **コードのパフォーマンス**
- [ ] メモリリークや非効率なループなど、パフォーマンスを低下させる可能性のある箇所はないか？
- [ ] 大量のデータを扱う場合、データ構造やアルゴリズムは適切に選択されているか？
## **コードのセキュリティ**
- [ ] 入力値の検証とエラーハンドリングは適切に行われているか？
- [ ] SQLインジェクションやクロスサイトスクリプティング(XSS)など、一般的なセキュリティ問題を防ぐための対策が施されているか？
## **テスト**
- [ ] 単体テストが全ての重要な関数やメソッドに対して書かれているか？
- [ ] テストケースは、正常系だけでなく異常系もカバーしているか？
- [ ] テストは自動化されているか？
## **ドキュメンテーション**
- [ ] 関数やクラス、モジュールに対するドキュメンテーションは適切に書かれているか？
- [ ] 複雑なアルゴリズムや重要なビジネスロジックについては、コメントで詳細に説明されているか？
うーん、長い！
全体的にもっともらしいことは言っているのですが、
正直こんなの確認してたら大変ですね。
「忘れる」し、実用的ではありません。
個人的には7つの大枠でまとめられたら使えそうに感じます。
ということで、自分なりに使えそうなチェックリストに変換します。
自分なりのチェックリストへ
ChatGPTの内容を踏まえて、コードレビューに使えそうなチェックリストに変換しました。
## **テスト確認**
- [ ] 単体テストが全ての重要な関数やメソッドに対して書かれているか？
- [ ] テストケースは、正常系だけでなく異常系もカバーしているか？
## **品質確認**
- [ ] 変数名、関数名は適切に命名されていて、その役割を理解できるか？
- [ ] 一つの関数やメソッドは一つの機能のみを担当しているか？（単一責任の原則）
- [ ] コードのリファクタリングが必要な箇所はないか？（重複コード、長すぎる関数、大きなクラスなど）
## **セキュリティー確認**
- [ ] 入力値の検証とエラーハンドリングは適切に行われているか？
- [ ] SQLインジェクションやクロスサイトスクリプティング(XSS)など、一般的なセキュリティ問題を防ぐための対策が施されているか？
- [ ] 機密情報や個人データの扱いに関するセキュリティポリシーが遵守されているか？
## **パフォーマンス確認**
- [ ] メモリリークや非効率なループなど、パフォーマンスを低下させる可能性のある箇所はないか？
- [ ] 大量のデータを扱う場合、データ構造やアルゴリズムは適切に選択されているか？
## **ドキュメント確認**
- [ ] 関数やクラス、モジュールに対するドキュメンテーションは適切に書かれているか？
- [ ] 複雑なアルゴリズムや重要なビジネスロジックについては、コメントで詳細に説明されているか？
これぐらいの量でしたら十分そうです。
大きなくくりとしては5つ。
テスト・品質・セキュリティー・パフォーマンス・ドキュメント。
品質確認する際はやはり、抑えておきたいのが「変数名」と「単一責任の原則」あたりでしょうか。
これを使えば、基本的な項目は拾えそうです。
拾えない内容
冒頭で紹介した記事にて「rollback～」というレビュー内容があります。
現在のチェックリストでは、なかなか拾えない項目です。
ということで色々と聞いてみた結果、次のような項目を増やせと指摘されました。
## **ファイル確認**
- [ ] ファイルが適切な場所に配置されているか？
- [ ] ファイル名はわかりやすく、役割を反映しているか？
- [ ] ファイルの拡張子は適切か？（例: **`.txt`**、**`.csv`**、**`.json`**）
- [ ] ファイルの内容やフォーマットは正しいか？
- [ ] ファイルの読み取り権限やアクセス制御は適切に設定されているか？
- [ ] ファイルのバックアップやバージョン管理が適切に行われているか？
- [ ] ファイルの関連性や依存関係が適切に管理されているか？
- [ ] ファイルの変更に関連するロールバック手順が文書化されているか？
- [ ] ファイルの変更をロールバックするための手順やコマンドが定義されているか？
- [ ] ロールバック手順が正常に動作し、ファイルが以前の状態に戻るかどうかをテストする方法があるか？
なるほど。
確かにファイル確認という大枠は必要そうです。
結構ファイル関係ってバグりやすいですし、
レビュー時に軽視されがちなので、
これはチェックリストに入れたほうが良いですね。
最終的に出来上がったフォーマット
ここまでの内容を全てまとめて、次のようなフォーマットが完成しました。
最終フォーマット
## **テスト確認**
- [ ] 単体テストが全ての重要な関数やメソッドに対して書かれているか？
- [ ] テストケースは、正常系だけでなく異常系もカバーしているか？
## **品質確認**
- [ ] 変数名、関数名は適切に命名されていて、その役割を理解できるか？
- [ ] 一つの関数やメソッドは一つの機能のみを担当しているか？（単一責任の原則）
- [ ] コードのリファクタリングが必要な箇所はないか？（重複コード、長すぎる関数、大きなクラスなど）
## **セキュリティー確認**
- [ ] 入力値の検証とエラーハンドリングは適切に行われているか？
- [ ] SQLインジェクションやクロスサイトスクリプティング(XSS)など、一般的なセキュリティ問題を防ぐための対策が施されているか？
- [ ] 機密情報や個人データの扱いに関するセキュリティポリシーが遵守されているか？
## **パフォーマンス確認**
- [ ] メモリリークや非効率なループなど、パフォーマンスを低下させる可能性のある箇所はないか？
- [ ] 大量のデータを扱う場合、データ構造やアルゴリズムは適切に選択されているか？
## **ドキュメント確認**
- [ ] 関数やクラス、モジュールに対するドキュメンテーションは適切に書かれているか？
- [ ] 複雑なアルゴリズムや重要なビジネスロジックについては、コメントで詳細に説明されているか？
## **ファイル確認**
- [ ] ファイルが適切な場所に配置されているか？
- [ ] ファイルの内容やフォーマットは正しいか？
- [ ] ロールバック手順が正常に動作し、ファイルが以前の状態に戻るかどうかをテストする方法があるか？
シミュレーションを行ってもらう
今までの内容を踏まえて、ChatGPTにレビューを実践してもらいました。
次はレビュー対象のサンプルコードです。
class User:
def __init__(self, name, age, email):
self.name = name
self.age = age
self.email = email
def get_email(self):
return self.email
def set_email(self, new_email):
self.email = new_email
return self.email
def get_age(self):
return self.age
def birthday(self):
self.age += 1
return self.age
実際にレビューを行ってもらった結果が以下のようになります。
全体的に見て、コードは簡潔で機能も明確に定義されています。以下にいくつかのポイントを挙げます。
1. クラス名と関数名は適切に命名されており、その目的を適切に反映しています。これによりコードの可読性が高まっています。
2. 各メソッドは一つの機能のみを担当しています。これは単一責任の原則に従っています。
3. コードは簡潔であり、不必要な冗長性はありません。これによりコードのメンテナンスが容易になります。
4. Userクラスはカプセル化の原則に基づいてデータと操作を一緒に保持しています。しかしながら、クラスが他のクラスを継承したり、多態性を示すような振る舞いは特にありません。必要であればオブジェクト指向の他の原則も考慮してみてください。
一方で、以下の2点について改善をお勧めします。
1. 各関数には、その目的を説明するコメント（ドキュメンテーション）が付けられていません。これは可読性とメンテナンス性を向上させるために重要です。例えば、**`birthday`**関数は年齢を増やすだけでなく、新しい年齢を返すということがコメントによって明示されているとより良いでしょう。
2. このコードにはテストが含まれていません。Userクラスとそのメソッドの動作を確認するためのテストを書くことで、今後のコードの変更が予期しない副作用を引き起こすことを防げます。
前半で褒めて、後半で指摘するというのは私もよくやる手法です。
ただ、こんなにレビュー文章が長いと「書き手」も「読み手」も時間が食われて煩わしいとは思います。
もっと簡潔に指摘するほうが良さそうです。
そこらへんを今後は深ぼったほうが良さそうですね。
今後
今後はこのチェックリストをベースに、自分なりのコードレビューのスキルを上げていきたいと思います。
また、簡潔にレビューする方法とかも学んでいきたいですね。