この記事はNuco Advent Calendar 2024の17日目の記事です。
0.はじめに
本記事は、以下の目的で作成しました。
- 初心者の方: 実務経験を積み始めた際、上司にコードレビューを依頼する前のチェックシートとして活用いただけます。
- 中堅からベテランの方: 後輩への指導の際に、備忘録や振り返りの参考資料として活用いただけます。
コードはPythonで書かれていますが、内容はコーディング全般に通じるコードレビューの観点を網羅することを目指しています。
それでは、さっそくコードレビュー時のチェックポイントを確認していきましょう。
弊社Nucoでは、他にも様々なお役立ち記事を公開しています。よかったら、Organizationのページも覗いてみてください。
また、Nucoでは一緒に働く仲間も募集しています！興味をお持ちいただける方は、こちらまで。
目次
0.はじめに
1.基本的なコーディング規約の遵守
2.コードの読みやすさとメンテナンス性
3.安全なコーディング
4.最適化とパフォーマンス
5.テストの充実とPR作成に関して
6.APIレビュー
7.まとめ
1.基本的なコーディング規約の遵守
規約を定める本質は、コードの品質や保守性を向上させ、最終的にはプロダクトの価値を高めることにあります。
「なぜ規約がこれほど細かく定められているのか？」
「なぜレビュー時に何度も口酸っぱく指摘されるのか？」
と感じたときは、次のことを思い出してください。
規約を守ることは、チームやプロジェクトが提供する価値に大きく貢献しているということを。
| 項目番号 | チェックポイント |
|---|---|
| 1-1 | 変数・関数の命名ルールが遵守されているか |
| 1-2 | 命名が明確でわかりやすいか |
| 1-3 | インデントやスペースの一貫性が保たれているか |
| 1-4 | デバッグ用のコードが紛れていないか |
| 1-5 | ドキュメンテーションの記述は適切か |
| 1-6 | 定数の定義と利用は適切か |
| 1-7 | コメント管理は適切か |
1-1.変数・関数の命名ルールが遵守されているか
各プログラミング言語には、それぞれ推奨される命名規約があります。
使用している言語の規約に従い、適切に命名するよう心がけましょう。
例えば、Pythonでは「変数名」にスネークケース（snake_case）が推奨されており、ケバブケース（kebab-case）は非推奨とされています。
スネークケース：snake_case
# _（アンダースコア）で単語を区切る
user_name = "John"
ケバブケース：kebab-case
Pythonでは非推奨
# ❌ -（ハイフン）で単語を区切る
user-name = "John"
# ⭕️ Gitのブランチ名ではケバブケースが支持されているようです
feature-creat-user
アッパーキャメルケース（別名パスカルケース）：CamelCase
# 単語の最初の文字をすべて大文字で単語を区切る
class UserProfile:
pass
ロワーキャメルケース：camelCase
# 最初の単語を小文字、以降の単語の最初の文字を大文字で単語を区切る
def userLogin():
pass
1-2.命名が明確でわかりやすいか
変数や関数の命名は、コードの可読性と保守性に大きな影響を与えます。
たとえ自分一人で開発している場合でも、aやbのような意味が不明瞭な名前を避け、意図が明確に伝わる名前を付けることが重要です。
良い命名をすることは簡単ではありませんが、次の点に注意することで改善できます。
命名の基本ルール
-
意味が伝わる名前を付ける
- 何を表しているのか、または何をするのかがすぐに理解できるようにしましょう。
- 曖昧な名前ではなく、具体的でわかりやすい名前を選びます。
-
情報の過不足を避ける
- 名前が短すぎて意味が伝わらないことや、逆に冗長すぎて混乱を招くことを避けます。
- 少し長くても「コードの意図が理解できる」名前にすることを優先してください。
-
文法的に正しい識別子名を使う
- 基本的に識別子（変数名や関数名）は、文法上正しい形式にすることが好まれます。例えば、関数名が動詞と目的語の組み合わせになっているか、変数名が意味のある名詞になっているかを確認しましょう。
これにより、コードは将来の自分やチームのメンバーにとっても読みやすく、保守しやすいものになります。
# ❌️ クラスの役割が不明確、意味が曖昧
class Processor:
pass
class Sender:
pass
# ⭕️ クラスの役割が明確になり、どのような振る舞いが期待されているのかわかる
class PaymentProcessor: # 支払処理を行うクラスであることが一目で分かります
pass
class EmailSender: # メールを送信する機能を持つクラスであることが明確になります
pass
組み込み関数を変数名や関数名として代入していないか
組み込み関数を変数名や関数名として使用するのは避けましょう。その理由は次の通りです。
- 元の組み込み関数の機能が利用できなくなるため、予期しないバグを引き起こす可能性があります。
- そもそもエラーとして扱われる言語もあり、意図しない挙動につながります。
- 他の開発者がコードを読んだ際、混乱を招き、コードの可読性が低下します。
（特にPythonでは、組み込み関数名を変数や関数名として再定義することが可能です。そのため、以下のような例を挙げます。）
# ❌️ 組み込み関数 "list" を変数名として使用
list = [1, 2, 3, 4]
# 組み込み関数 "list" を使用しようとする
numbers = list(range(5))
print(numbers)
# 出力結果
TypeError: 'list' object is not callable
# ⭕️ 組み込み関数 "list" をそのまま利用し、別名を使う
my_list = [1, 2, 3, 4]
# 組み込み関数 "list" を正常に利用
numbers = list(range(5))
print(numbers)
# 出力結果
[0, 1, 2, 3, 4]
参考資料：
【日本人エンジニア必携】英語命名規則の決定版
可読性の高いコードを書くための実践ガイド
1-3.インデントやスペースの一貫性が保たれているか
# ❌️ タブとスペースが混在しPythonはこれをエラーとして扱うことがある
def calculate_sum(a, b):
if a>0: # タブでインデント
return a+b # 混在したインデントとスペース不足
else:
return 0 # スペース数が異なる
# ⭕️ インデント幅を統一することでコードの構造がわかりやすくなり、エラーを防ぐ
def calculate_sum(a, b):
if a > 0: # スペースで統一
return a + b # 演算子の周りにスペースを追加
else:
return 0 # 一貫したインデント幅（4スペース）
参考資料：
PEP:8|Python コードのスタイルガイド「インデント」
1-4.デバッグ用のコードが紛れていないか
# ❌️ デバッグ用のコードが残っている
def calculate_total(price, quantity):
total = price * quantity
print(f"DEBUG: Calculated total = {total}")
return total
# ⭕️ print文の削除
def calculate_total(price, quantity):
total = price * quantity
return total
# ⭕️ ロギングを使用
import logging
logging.basicConfig(level=logging.INFO)
def calculate_total(price, quantity):
total = price * quantity
logging.debug(f"DEBUG: Calculated total = {total}") # デバッグ時のみ出力
return total
# デバッグモードで実行
logging.getLogger().setLevel(logging.DEBUG)
calculate_total(10, 5) # DEBUG: Calculated total = 50
# 制御機能によりDEBUGの設定を行う
1-5.ドキュメンテーションの記述は適切か
ドキュメンテーションとは、記述対象のクラスや関数がどのような振る舞いをするのかを、仕様書や設計書としてまとめることを指します。
ただ単に挙動を漫然と記述するのではなく、「どのような振る舞いをするか」だけでなく、「どのような考えでこのクラスや関数を書いたのか」が伝わる文書を作成することを心がけましょう。
ドキュメンテーションのメリット
- 設計時の抜け漏れを減らす
- 設計品質の向上
- テストケースの設計が行いやすくなる
明確で伝わりやすいドキュメンテーションは、開発プロセス全体を効率化し、チーム全体の理解を深める助けになります。
# ❌️ どのような振る舞いが期待されているのかがわからない
def fetch_data():
"""
データを取得する
"""
pass
# ⭕️ 明確に詳細な説明がなされている
def fetch_data(source_url):
"""
指定されたURLからデータを取得する。
Args:
source_url (str): データを取得する対象のURL
Returns:
dict: 取得したデータを格納した辞書形式のオブジェクト
Raises:
ValueError: URLが無効な場合に発生
ConnectionError: サーバーとの接続に失敗した場合に発生
"""
pass
参考資料：
ドキュメンテーションコメントからプログラミングを考える / 『エンジニアのためのJavadoc再入門講座』を読んだ
ドキュメンテーション 【documentation】 文書化 / ドキュメント化
1-6.定数の定義と利用は適切か
定数とは、一度定義すると値が変わらないデータを指します。その用途は数値に限らず、文字列や特定のパス、設定値など、変更が許されないあらゆる値に広がります。たとえば以下のようなケースが挙げられます。
- 税率や円周率（数値）
- デフォルトの設定値やユーザー権限（文字列・フラグ）
- ファイルパスやURL（パス情報）
定数として扱うメリット
-
意図の明確化
- 値が何を意味するのかが明示され、コードを読む人がすぐに理解できます。
-
保守性の向上
- 値を変更する際に、定数を修正するだけで済むため、エラーのリスクが減ります。
-
再利用性の向上
- 定数を共有することで、一貫性のあるコードが書けます。
マジックナンバーとはコード内に直接記述された意味が不明確な数値や文字列を指します。
読んだだけではその値が何を意味しているのか分からないため、コードの意図を理解しにくくする原因になります。
マジックナンバーを避けるべき理由
-
意図がわかりにくい
- なぜその値が使われているのかが明確でないため、コードの意味を理解するのに余計な時間がかかります。
-
保守性の低下
- 同じ値を複数箇所で使用している場合、修正の手間が増え、修正漏れの原因になります。
-
再利用性の低下
- 再利用や他のプロジェクトへの移植が難しくなります。
定数はコード全体の意図を伝えやすくし、マジックナンバーを排除することで、コードの可読性、保守性を大きく向上させます。数値だけでなく、文字列や設定値にも意識を広げ、適切な命名を心がけましょう。
# ❌️ 定数が直接記述されている
def calculate_circle_area(radius):
return radius * radius * 3.14159
# ⭕️ 関数の外にPIの値を定めて、関数の外を参照している
PI = 3.14159
def calculate_circle_area(radius):
return radius * radius * PI
参考資料：
定数 【constant】 const / 常数
1-7.コメント管理は適切か
「美しいコード」はコメントが少ないことを理想としますが、初心者や実務経験の浅いエンジニアにとっては、適切なコメントを残すことが重要なステップです。必要なコメントと不要なコメントを区別して記述しましょう。
コメント管理のポイント
- 他のメンバーがコードを理解できるか。
- コードだけで意図が伝わるか。
- コメントが冗長になっていないか。
これらを心がけることで、コメントの価値を最大化し、チームでの開発効率を向上させることができます。
不要なコメントや古いコメントが削除されていない
# ❌️ 不要なコメントや古いコメントが削除されていない
def calculate_sum(a, b):
# ここでリストを初期化するが、現在は不要
# items = []
if a > 0:
return a + b # この部分は修正予定
else:
return 0 # 計算失敗
# ⭕️ 不要なコメントの削除
def calculate_sum(a, b):
if a > 0:
return a + b
else:
return 0
アノテーションコメントの管理はできているか
アノテーションコメントとは、該当コードの状態や意図を注釈（annotation）として記述するコメントを指します。
このコメントは、コードを読む他の開発者に対して、コードがどのような状態であるのか、何を意図しているのかを明確に伝える役割を果たします。
# ❌️ コメントが明確でない
# TODO: 何かを追加する
def calculate_area(length, width):
# 値の確認をするかも？
return length * width
# ⭕️ コメントが明確
# TODO: 引数のバリデーションを追加する（2024-11-15まで）
def calculate_area(length, width):
# 長さと幅が正の数であることを確認
return length * width
# ❌️ コメントが明確でない
# FIXME: なぜか負の値になる
def discounted_price(price, discount):
return price - discount # 割引率が正しいか確認する必要あり
# ⭕️ コメントが明確
# FIXME: 割引率が100%を超えると負の値になるバグを修正（優先度: 高）
def discounted_price(price, discount):
# 割引率が価格の100%を超えないようにチェックを追加する
return price - discount
参考資料：
TODO: 以外のアノテーションコメントをまとめた
『開発プロフェッショナル 〜コメントの流儀〜』コメントの考え方・実践ルール・コメント駆動開発について
コメントすべきこととすべきでないこと
2.コードの読みやすさとメンテナンス性
| 項目番号 | チェックポイント |
|---|---|
| 2-1 | 1つの関数が1つの役割を担うように設計しているか |
| 2-2 | グローバル変数に依存していないか |
| 2-3 | 早期リターンによるネストの回避はしているか |
| 2-4 | クラスの適切な利用をしているか |
| 2-5 | モジュールの分割と再利用性はできているか |
| 2-6 | 複雑な条件式の簡略化はなされているか |
| 2-7 | 最適なループになっているか |
| 2-8 | デッドコードがないか |
| 2-9 | 最新のドキュメンテーションに更新しているか |
| 2-10 | 既存の実装との整合性はとれているか |
2-1.1つの関数が1つの役割を担うように設計しているか
一関数一機能の原則を守りましょう。これは一つの関数が一つの責任だけを担い、他の処理に干渉しないようにすることを意味します。
この原則を守ることで、関数がシンプルで理解しやすくなり、メンテナンス性が向上します。
一関数一機能のメリット
-
汎用性の向上
- 独立した処理として関数が切り出されているため、他のコードでも再利用しやすくなります。
-
テストの容易さ
- 単体テストを簡単に実装でき、各機能を個別に検証することが可能です。
-
デバッグがしやすい
- 問題が発生した場合、責任範囲が限定されるため、原因を特定しやすくなります。
-
可読性の向上
- 関数が短くなることで、コードを読む人にとって理解しやすい構造になります。
# 入力データ
order = {
"items": [
{"price": 10.0, "quantity": 2}, # 合計: 20.0
{"price": 20.0, "quantity": 1}, # 合計: 20.0
{"price": 5.0, "quantity": 3}, # 合計: 15.0
]
}
# ❌️ 1つの関数が複数の機能を持っている
def main(order):
# 注文情報をバリデーション
if not isinstance(order, dict) or 'items' not in order:
raise ValueError("Invalid order format")
# 合計金額を計算
total = sum(item['price'] * item['quantity'] for item in order['items'])
# 注文確認メールを送信
print(f"Sending email: Your total is ${total}")
return total
# ⭕️ 一関数一機能
def validate_order(order):
if not isinstance(order, dict) or 'items' not in order:
raise ValueError("Invalid order format")
def calculate_total(order):
return sum(item['price'] * item['quantity'] for item in order['items'])
def send_confirmation_email(total):
print(f"Sending email: Your total is ${total}")
def main(order):
validate_order(order)
total = calculate_total(order)
send_confirmation_email(total)
return total
2-2.グローバル変数に依存していないか
グローバル変数への依存は避けましょう。グローバル変数を使用すると、以下の問題が発生しやすくなります。
グローバル変数依存のデメリット
-
コードの再利用性が低下する
- グローバル変数が特定の状態に依存している場合、そのコードを別の場所で再利用することが難しくなります。
-
デバッグが難しくなる
- グローバル変数が意図せず変更されると、予測不可能なバグが発生し、原因を特定しづらくなります。
-
可読性の低下
- 関数やクラスがどの変数に依存しているのかが明示されないため、コードを読む他の開発者が混乱しやすくなります。
# ❌️ どこからでもsum_scoreに参照できる
sum_score = 0
def process_scores(students):
for student_name, score in students:
global sum_score
sum_score += score
return sum_score
# サンプルデータ
students = [("Alice", 85), ("Bob", 90), ("Charlie", 75)]
print(process_scores(students))
print(process_scores(students))
# 出力結果
250
500 # 1つ前の処理に引きずられてしまう。
# ⭕️ グローバル変数を使用しない
def process_scores(students):
"""各生徒の点数を正確に処理し、合計点を計算"""
sum_score = 0
for student_name, score in students:
sum_score += score
return sum_score
# サンプルデータ
students = [("Alice", 85), ("Bob", 90), ("Charlie", 75)]
print(process_scores(students))
print(process_scores(students))
# 出力結果
250
250
参考資料：
副作用と参照透過性と冪等性を理解して、テストでドメインを磨く
参照透過性について(Haskell)
2-3.早期リターンによるネストの回避はしているか
# ❌️ ネストが深い
def categorize_age(age):
if age >= 0: # 正しい年齢か確認
if age < 6:
return "幼児"
else:
if age < 13:
return "13歳未満"
else:
if age < 20:
return "20歳未満"
else:
return "20歳以上"
else:
return "無効な年齢"
# ⭕️ 早期のリターンでネストを浅くする
def categorize_age(age):
if age < 0: # 無効な年齢
return "無効な年齢"
if age < 6:
return "幼児"
if age < 13:
return "13歳未満"
if age < 20:
return "20歳未満"
return "20歳以上"
参考資料：
ガード (プログラミング)
早期リターンを書こう
2-4.クラスの適切な利用をしているか
クラスの属性を直接参照することは、データの変更後の追跡が難しくなります。また、他のコードでその属性を直接参照している場合、変更が広範囲に影響を及ぼす可能性があります。
データの整合性を保つための原則
-
属性への直接アクセスを避ける
- クラスの属性には、直接参照するのではなく、アクセサメソッド（getter、setter）やプロパティを通じてアクセスするようにしましょう。これにより、変更の影響範囲を小さく抑えられます。
-
コードの重複をまとめる
- 重複した処理があると、修正時に複数箇所を変更する必要があり、保守性が低下します。類似した処理は共通化して再利用可能な形にしましょう。
-
ロジックをメソッド化する
- ロジックをメソッド化することで、モジュールとして切り出して使用できるため、汎用性が向上します。
# ❌️ 属性を直接参照している
class User:
def __init__(self, name, age):
self.name = name # 直接参照可能
self.age = age
# 使用例
user = User("Alice", 30)
user.age += 1 # 属性を直接変更
# ⭕️ メソッドやプロパティで属性を制御している
class User:
def __init__(self, name, age):
self._name = name
self._age = age
@property # プロパティで制御
def name(self):
return self._name
@property
def age(self):
return self._age
def get_older(self):
self._age += 1
# ❌️ 似た処理を他の場面で使用できないために再利用性が低い
# ❌️ 新しい機能を追加する際にコード全体を修正する必要があり拡張性に乏しい
# ❌️ コードが重複しているため保守性が低下している
# 3つのデータセットの平均値を計算する
data1 = [1, 2, 3]
data2 = [4, 5, 6]
data3 = [7, 8, 9]
avg1 = sum(data1) / len(data1)
avg2 = sum(data2) / len(data2)
avg3 = sum(data3) / len(data3)
print(f"Average 1: {avg1}")
print(f"Average 2: {avg2}")
print(f"Average 3: {avg3}")
# ⭕️ クラスでデータセットを管理
# ⭕️ ロジックをメソッドとして実装することで、コードがモジュール化され拡張しやすくなる
class Dataset:
def __init__(self, data):
self._data = data
@property
def data(self):
"""データの読み取り用プロパティ"""
return self._data
@property
def average(self):
"""データの平均値を計算"""
if len(self._data) == 0:
raise ValueError("データが空です")
return sum(self._data) / len(self._data)
# データセットを作成
datasets = [
Dataset([1, 2, 3]),
Dataset([4, 5, 6]),
Dataset([7, 8, 9]),
]
# 各データセットの平均を出力
for i, dataset in enumerate(datasets, 1):
print(f"Average {i}: {dataset.average}")
参考資料：
Pythonの「クラス」を理解、オブジェクト指向プログラミングの基本を押さえる
初心者でもわかるPython「クラス」入門
2-5.モジュールの分割と再利用性はできているか
モジュールの分割を行うことで、以下のような利点が得られます。
モジュール分割のデメリット
-
モジュール単位での開発が可能
- コードを機能ごとに分割することで、チームメンバーが独立して作業できる環境を構築できます。
-
問題の特定と改修が容易
- 問題が発生した際、特定の機能に関係するモジュールだけを改修すれば済むため、影響範囲を最小限に抑えられます。
-
再利用性の向上
- 分割されたモジュールは、他のプロジェクトやスクリプトからインポートして利用できるため、開発効率が向上します。
# ❌ すべてのコードが1つのファイルに集中
# app.py
def calculate_area(length, width):
return length * width
def send_email(recipient, message):
print(f"Sending email to {recipient}: {message}")
def main():
area = calculate_area(10, 5)
print(f"Area: {area}")
send_email("example@example.com", f"The area is {area}")
main()
# ⭕️ 機能ごとにモジュールを分割
project/
|-- app.py
|-- libs/
|-- calculations.py
|-- notifications.py
def calculate_area(length, width):
return length * width
def send_email(recipient, message):
print(f"Sending email to {recipient}: {message}")
from libs.calculations import calculate_area
from libs.notifications import send_email
def main():
area = calculate_area(10, 5)
print(f"Area: {area}")
send_email("example@example.com", f"The area is {area}")
if __name__ == "__main__":
main()
2-6.複雑な条件式の簡略化はなされているか
一文で条件式を書きたくなる気持ちは理解できます。しかし、条件式を見直してみましょう。
条件を細分化し、小さな条件として独立させることで、次のようなメリットが得られます。
小さな条件式のメリット
-
可読性の向上
- 複雑な条件を分解することで、コードを読む人が意図を理解しやすくなります。
-
再利用性の向上
- 条件を関数や変数に分けることで、同じロジックを他の箇所でも使えるようになります。
-
デバッグやテストの容易さ
- 個々の条件を個別にテストできるため、バグの特定がしやすくなります。
from datetime import datetime, timedelta
class User:
def __init__(self, name, role, is_verified, last_login_at):
self._name = name
self._role = role
self._is_verified = is_verified
self._last_login_at = last_login_at
@property
def name(self):
return self._name
@property
def role(self):
return self._role
@property
def is_verified(self):
return self._is_verified
@property
def last_login_at(self):
return self._last_login_at
# ユーザー作成
user = User("Alice", "admin", True, datetime(2024, 12, 10))
# ❌️ 各条件の再利用が難しく、条件節が長い
if user.role == "admin" and user.is_verified and user.last_login_at > datetime.now() - timedelta(days=30):
print(f"{user.name} is a valid user")
# ⭕️ 各条件を変数に分けて命名
# ⭕️ シンプルで分かりやすい条件文
from datetime import datetime, timedelta
class User:
# (上記と同じため)省略
@property
def is_admin(self):
return self.role == "admin"
@property
def is_active(self):
recent_threshold = datetime.now() - timedelta(days=30)
return self.is_verified and self.last_login_at > recent_threshold
# ユーザー作成
user = User("Alice", "admin", True, datetime(2024, 12, 10))
if user.is_admin and user.is_active:
print(f"{user.name} is a valid user")
2-7.最適なループになっているか
ループ処理を最適化することは、コードの可読性や保守性に大きく影響します。不適切なループ構造は冗長でバグを招きやすく、さらにパフォーマンスにも悪影響を与える場合があります。
Pythonには、繰り返し処理を簡潔かつ効率的に記述できる便利な機能が備わっています(下で紹介しているenumerateやzipなど)。使わなくても実装はできますが、利用するとより良いコードにすることができます。
# ❌️ インデックスを手動で管理
fruits = ["apple", "banana", "cherry"]
index = 0
for fruit in fruits:
print(f"{index}: {fruit}")
index += 1
# ⭕️ enumerateを使用
fruits = ["apple", "banana", "cherry"]
for index, fruit in enumerate(fruits):
print(f"{index}: {fruit}")
# ❌️ インデックスで複数のリストを操作
names = ["Alice", "Bob", "Charlie"]
ages = [25, 30, 35]
for i in range(len(names)):
print(f"{names[i]} is {ages[i]} years old.")
# ⭕️ zipを使用
names = ["Alice", "Bob", "Charlie"]
ages = [25, 30, 35]
for name, age in zip(names, ages):
print(f"{name} is {age} years old.")
参考資料：
最適化しやすいソースコードの書き方
2-8.デッドコードがないか
実行されないコード（デッドコード）は、以下のような問題を引き起こすため、削除することを心がけましょう。
デッドコードのデメリット
-
不要なメモリ消費の原因
- 実行されないコードがメモリに残る場合、リソースが無駄に消費されます。
-
可読性の低下
- 実行されないコードが混在すると、他の開発者がコードを読む際に混乱を招き、意図を理解するのが難しくなります。
-
メンテナンスの負担増
- デッドコードが残っていると、コードの管理が複雑になり、不要な修正や確認が発生する可能性があります。
# ❌️ 実行されないコードが削除されていない
def check_value(value):
if value > 0:
print("Value is positive.")
return
else:
print("Value is non-positive.")
return
print("hogehoge") # 実行されないコード
# ⭕️ 実行されないコードの削除
def check_value(value):
if value > 0:
print("Value is positive.")
return
else:
print("Value is non-positive.")
return
2-9.最新のドキュメンテーションに更新しているか
コードが更新されてもドキュメンテーションが古いままだと、以下のような問題が発生します。
古いドキュメンテーションのデメリット
-
混乱の原因
- 開発者がドキュメンテーションを信頼して作業を進めた場合、実際のコードと矛盾しているとミスや誤解を招きます。
-
メンテナンスの負担増
- 古いドキュメンテーションが残っていると、どれが正しい情報かを確認する手間が増え、効率が低下します。
-
チームの連携に支障
- 最新情報が共有されていないと、特に大規模なプロジェクトで開発者間の連携が崩れる可能性があります。
2-10.既存の実装との整合性はとれているか
一貫性を持たせるためには、チームやプロジェクトのコーディング規約を明確にし、それに従うことが重要です。一貫したスタイルのコードは、読みやすさだけでなく、将来的なメンテナンス性も向上させます。
局所的な最適化を意識する際でも、全体の一貫性を優先しましょう。
一貫性のないコードの一デメリット
-
可読性の低下
- 開発者ごとに異なるスタイルで書かれたコードは、理解しにくくなり、チーム全体の生産性が低下します。
-
保守性の低下
- 修正や追加を行う際に、スタイルが統一されていないとミスが増え、保守コストが高くなります。
# ❌️ 命名を既存の規則に合わせていない
def fetch_user_by_id(user_id):
pass
def fetch_all_users():
pass
def get_users_by_email(email):
pass
# ⭕️ 命名を既存の規則に合わせる
def fetch_user_by_id(user_id):
pass
def fetch_all_users():
pass
def fetch_users_by_email(email):
pass
コードを書く際には、同じ目的を達成する異なる方法があったとしても、統一されたスタイルを採用することが重要です。
コード全体で統一感を保つことで、他の開発者がコードを理解しやすくなり、ミスや誤解を減らすことができます。
# ❌️ コードに統一感が無い
# ① 大小の関係を記述
b > a
# ② 同じ意味だが順序が逆
a < b
統一感を持たせるために、a < b の形式を採用すると決めた場合のコードです。
# ⭕️ a < b の形式を統一として採用
if a < b:
print("a is less than b")
局所的に「効率的」または「読みやすい」と感じる方法を採用するのも大事ですが、コード全体の一貫性がそれ以上に重要です。一貫性を保つことで、チームメンバー全員がコードを理解しやすくなり、保守性も向上します。
# ❌️ 冗長な方法
# ① 直接リストを作成
numbers = [0, 1, 2, 3, 4]
# ② リスト内包表記を使用
numbers = [i for i in range(5)]
# ③ forループでappendを使用
numbers = []
for i in range(5):
numbers.append(i)
上記のコードでは、目的（[0, 1, 2, 3, 4]というリストを作成）が同じであるにもかかわらず、書き方がばらばらです。これにより、コードを読んだ他の開発者が「なぜ異なる書き方をしているのか？」と疑問に思い、読みづらく感じるでしょう。
以下のように、コード全体で一貫したスタイルを採用することが望ましいです。
# ⭕️ 小さな範囲でリストを生成する場合は直接記述 [0, 1, 2, 3, 4] も許容
# ⭕️ 冗長な方法（for ループで append）は非推奨
numbers = [i for i in range(5)]
3.安全なコーディング
| 項目番号 | チェックポイント |
|---|---|
| 3-1 | 例外処理の記述は適切か |
| 3-2 | ユーザー入力のバリデーションは行っているか |
| 3-3 | セキュアコーディングはできているか |
| 3-4 | 機密情報の管理は適切か |
| 3-5 | 結果を返し忘れたりしてしまうコードパスがないか |
| 3-6 | キャッシュの利用は適切か |
| 3-7 | 処理系依存の処理がないか |
3-1.例外処理の記述は適切か
エラーが発生した際には、システムを正常に終了させることが重要です。適切な例外処理を記述することで、予期しない動作やデータの破損を防ぎます。
また、エラー発生時にはログを記録しておくことで、デバッグや将来の対応がスムーズになります。
例外処理のポイント
-
適切な例外をキャッチする
- 発生する可能性のあるエラーを特定し、それに応じた適切な例外処理を記述しましょう。
-
システムを正常に終了させる
- エラー発生時には、リソースの解放や必要なクリーンアップ処理を行い、システムが安全に終了するようにします。
-
エラー内容をログに記録する
- エラーが発生した箇所やその詳細をログとして残すことで、問題の特定や修正が容易になります。
# ❌️ 詳細を記録しない
def read_file(file_path):
try:
with open(file_path, 'r') as file:
return file.read()
except Exception: # 全ての例外を握りつぶしている。ログも出さない。
pass
# ⭕️ エラー詳細をログに記録
import logging
# ログ設定
logging.basicConfig(level=logging.ERROR, format='%(asctime)s - %(levelname)s - %(message)s')
def read_file(file_path):
file = None
try:
file = open(file_path, 'r')
data = file.read()
return data
except FileNotFoundError as e:
logging.error(f"File not found: {file_path}, Error: {e}")
return f"Error: The file '{file_path}' was not found."
except Exception as e:
logging.error(f"An unexpected error occurred with file: {file_path}, Error: {e}")
return "Error: An unexpected error occurred."
finally:
# finallyブロックでリソースを確実に解放
if file:
file.close()
logging.info(f"File {file_path} closed successfully.")
# ファイル読み込みを試す
file_content = read_file("example.txt")
print(file_content)
参考資料；
try-catch のfinallyっていつ使うねん、ていう話
3-2.ユーザー入力のバリデーションは行っているか
バリデーション（validation）とは「検証」を意味し、開発者が想定した入力のみを許可するためのプロセスです。これにより、システムに安全かつ有効なデータだけが渡されるようになります。
バリデーションの確認ポイント
-
有効性
- データが期待する形式や型、範囲に適合しているか。
-
安全性
- 入力データが害意のあるものでないか。
# ❌ バリデーションを行わない
def calculate_discount(price, discount):
return price - (price * discount)
# ⭕️ バリデーションを行えている
def calculate_discount(price, discount):
# 値が数値であるかを確認
if not isinstance(price, (int, float)) or not isinstance(discount, (int, float)):
raise TypeError("Both price and discount must be numbers.")
# 値チェック
if discount < 0 or discount > 1:
raise ValueError("Discount must be between 0 and 1.")
if price < 0:
raise ValueError("Price must be a non-negative value.")
# 割引計算
return price - (price * discount)
引数の型を確認せず実行すると以下のようなエラーになります。
# ❌️ 必須引数のstr型でないものを渡した場合
def split_name(username):
return username.split(" ")
print(split_name(123))
# 出力結果
AttributeError: 'int' object has no attribute 'split'
# ⭕️ 引数の存在と型をチェック
def split_name(username):
if not isinstance(username, str):
raise TypeError("The 'username' argument must be a string.")
if not username.strip():
raise ValueError("The 'username' argument must not be empty or whitespace.")
return username.split(" ")
# 出力結果
# str型以外を渡す場合
TypeError: The 'username' argument must be a string.
# 空白文字列 " " を渡す場合
ValueError: The 'username' argument must not be empty or whitespace.
参考資料；
バリデーションとは？ データマネジメント用語をわかりやすく解説
セキュア・プログラミング講座(p.31 5.4.「入力の検証」)
3-3.セキュアコーディングはできているか
セキュアコーディングは、害意のあるユーザーからプログラムやアプリケーションを守るために不可欠な開発手法です。
セキュアコーディングの重要性
-
情報漏洩の防止
- 個人情報や機密データの流出を防ぐためには、堅牢なセキュリティ対策が必須です。
-
プログラムの信頼性向上
- セキュアなコードは、ユーザーからの信頼性を高め、運用上のトラブルを未然に防ぎます。
-
法的リスクの回避
- 情報漏洩が発生した場合、企業に多額の損害賠償や社会的信用の失墜をもたらす可能性があります。
SQLインジェクション対策
- SQLインジェクションとはSQL文の組み立て方法に問題がある場合、害意のある入力よってデータベースの不正利用を行われることです。
# ❌️ ユーザー入力を直接埋め込む
import sqlite3
def get_user_by_username(username):
connection = sqlite3.connect("database.db")
cursor = connection.cursor()
query = f"SELECT * FROM users WHERE username = '{username}'"
cursor.execute(query)
return cursor.fetchall()
# ⭕️ パラメータ化クエリを使用
import sqlite3
def get_user_by_username(username):
connection = sqlite3.connect("database.db")
cursor = connection.cursor()
query = "SELECT * FROM users WHERE username = ?"
cursor.execute(query, (username,))
return cursor.fetchall()
XSS対策
- XSS（クロスサイトスクリプティング）とは害意のあるユーザーがスクリプトを出力画面に埋め込むことで、本来のウェブページの上が罠のページとなっており、情報漏洩につながったり、利用者が意図しない挙動をすることです。
# ❌️ 直接埋め込む
from flask import Flask, request, render_template_string
app = Flask(__name__)
@app.route('/comment', methods=['GET', 'POST'])
def comment():
if request.method == 'POST':
user_input = request.form['comment']
return render_template_string(f"<h1>コメント</h1><p>{user_input}</p>")
return '''
<form method="post">
<input type="text" name="comment">
<button type="submit">送信</button>
</form>
```
#### 問題点:
- ユーザー入力 `user_input` をそのままHTMLに埋め込んでいるため、次のような悪意のあるスクリプトが実行される可能性があります。
**攻撃例**:
```html
<script>alert('XSS!')</script>
# ⭕️ エスケープ処理を適用
from flask import Flask, request, render_template_string
import html
app = Flask(__name__)
@app.route('/comment', methods=['GET', 'POST'])
def comment():
if request.method == 'POST':
user_input = request.form['comment']
safe_input = html.escape(user_input) # 入力をエスケープ
return render_template_string(f"<h1>コメント</h1><p>{safe_input}</p>")
return '''
<form method="post">
<input type="text" name="comment">
<button type="submit">送信</button>
</form>
'''
参考資料：
安全なウェブサイトの作り方 - 1.5 クロスサイト・スクリプティング
安全なウェブサイトの作り方 - 1.1 SQLインジェクション
3-4.機密情報の管理は適切か
秘匿情報は必ず隠し、追跡せず、アップロードしないようにしましょう。
APIキー、パスワード、個人情報などの秘匿情報が誤って外部に公開されると、セキュリティインシデントや重大な損害につながる可能性があります。
特に、「GitHubに秘匿情報をアップロードしてしまった」という状況は深刻です。このようなミスを防ぐために、以下の対策を徹底しましょう。
# ❌ ソースコードに秘密情報を直接記述
DATABASE_URL = "postgres://user:password@localhost:5432/mydatabase"
SECRET_KEY = "mysecretkey"
# .envファイルの作成
DATABASE_URL=postgres://user:password@localhost:5432/mydatabase
SECRET_KEY=mysecretkey
# ⭕️ .envファイルを読み込む
import os
from dotenv import load_dotenv
load_dotenv()
DATABASE_URL = os.getenv("DATABASE_URL")
SECRET_KEY = os.getenv("SECRET_KEY")
if not DATABASE_URL or not SECRET_KEY:
raise EnvironmentError("Required environment variables are not set")
# ⭕️ .envファイルをリポジトリに含めないよう、.gitignore に追加
# .gitignore
.env
3-5.結果を返し忘れたりしてしまうコードパスがないか
# ❌ それ以外のケースで結果を返さない
def check_value(value):
if value == "special":
return "Special"
elif value == "important":
return "Important"
# ⭕️ すべてのケースに対応
def check_value(value):
if value == "special":
return "Special"
elif value == "important":
return "Important"
return "Other"
# ❌️ 再帰関数で返り値が欠落
def factorial(n):
if n == 0:
return 1
factorial(n - 1) # 結果を返していない
# ⭕️ 結果を返す
def factorial(n):
if n == 0:
return 1
return n * factorial(n - 1)
3-6.キャッシュの利用は適切か
キャッシュは、アプリケーションのパフォーマンスを大幅に向上させる重要な仕組みです。
ただし、キャッシュの利用には注意が必要で、誤った実装や設計は、意図しないバグやパフォーマンス劣化につながる可能性があります。
このセクションでは、キャッシュを適切に利用するためのポイントをコード例とともに解説します。
キャッシュを利用すべきケースの判断
- 頻繁にアクセスされるデータで、計算や取得に時間がかかる場合。
- 外部APIなど、ネットワーク遅延が発生する処理の結果を保存する場合。
- データが短期間で頻繁に変更されない場合。
パフォーマンス向上
同じ計算を繰り返さず、キャッシュされた結果を再利用することで計算コストを削減。
# ❌ このコードは同じ計算を繰り返し行うため、計算コストが高くなります。
def fibonacci(n):
if n <= 1:
return n
return fibonacci(n - 1) + fibonacci(n - 2)
# 実行例
if __name__ == "__main__":
import time
start_time = time.time()
print(fibonacci(35)) # 35番目のフィボナッチ数
print(f"Execution time: {time.time() - start_time:.2f} seconds")
# 出力結果
9227465
Execution time: 0.95 seconds
# ⭕️ functools.lru_cacheを使用することで、同じ引数で呼び出された関数の結果をキャッシュし、計算コストを大幅に削減します。
from functools import lru_cache
@lru_cache(maxsize=None) # キャッシュサイズを制限しない
def fibonacci(n):
if n <= 1:
return n
return fibonacci(n - 1) + fibonacci(n - 2)
# 実行例
if __name__ == "__main__":
import time
start_time = time.time()
print(fibonacci(35)) # 35番目のフィボナッチ数
print(f"Execution time: {time.time() - start_time:.2f} seconds")
# 出力結果
9227465
Execution time: 0.00 seconds
キャッシュの導入でパフォーマンス改善
以下は、外部APIから取得したデータをキャッシュする例です。
ポイント:
-
cachetoolsを使用して簡単にキャッシュ機能を追加。 -
TTL (Time to Live)を設定して、古いデータが残らないようにする
import requests
from cachetools import cached, TTLCache
# キャッシュの設定: 最大サイズ100、TTL (Time to Live) 1時間
cache = TTLCache(maxsize=100, ttl=3600)
@cached(cache)
def get_weather_data(city: str) -> dict:
"""指定された都市の天気データを取得し、キャッシュする"""
url = f"https://api.weatherapi.com/v1/current.json?key=API_KEY&q={city}"
response = requests.get(url)
response.raise_for_status()
return response.json()
# 使用例
print(get_weather_data("Tokyo"))
print(get_weather_data("Tokyo")) # キャッシュから取得されるためAPIリクエストは発生しない
適切なキャッシュ無効化の実装
キャッシュが適切に無効化されない場合、古いデータを返すリスクがあります。たとえば、バックエンドでデータが更新された場合に、キャッシュが古くなる可能性を考慮します。
ポイント:
- データ更新時にキャッシュも適切に無効化。
- キャッシュキー（この場合は
user_id）に注意して無効化を実装。
from cachetools import cached, TTLCache
cache = TTLCache(maxsize=100, ttl=3600)
@cached(cache)
def get_user_data(user_id: int) -> dict:
"""ユーザーデータを取得する"""
# 擬似的なデータベースクエリ
return {"id": user_id, "name": "John Doe"}
def update_user_data(user_id: int, new_data: dict):
"""ユーザーデータを更新し、キャッシュをクリアする"""
# データベースの更新処理 (擬似的)
print(f"Updating user {user_id} with {new_data}")
# キャッシュを無効化
cache.pop(user_id, None)
# 使用例
print(get_user_data(1)) # キャッシュに保存
update_user_data(1, {"name": "Jane Doe"})
print(get_user_data(1)) # キャッシュが無効化されているため新しいデータを取得
キャッシュのモニタリングとデバッグ
キャッシュのヒット率をモニタリングすることで、効果を確認し問題を特定できます。
ポイント:
- ヒット率が低い場合、キャッシュの粒度や設定を見直す。
- ログを活用してキャッシュの動作を確認。
from cachetools import cached, TTLCache
# キャッシュの初期化
cache = TTLCache(maxsize=100, ttl=3600)
# キャッシュ統計用のカウンタ
cache_stats = {"hits": 0, "misses": 0}
def log_cache_statistics():
"""キャッシュの統計情報を出力する"""
print(f"Cache hits: {cache_stats['hits']}, misses: {cache_stats['misses']}")
@cached(cache)
def get_data(key: str) -> str:
"""データを取得し、キャッシュに保存"""
if key in cache:
cache_stats["hits"] += 1
else:
cache_stats["misses"] += 1
return f"Data for {key}"
# キャッシュ操作の例
print(get_data("key1")) # ミス
log_cache_statistics()
print(get_data("key1")) # ヒット
log_cache_statistics()
print(get_data("key2")) # ミス
log_cache_statistics()
3-7.処理系依存の処理がないか
プログラムが特定の処理系（言語の実装や環境）に依存する場合、予期しない動作が発生する可能性があります。
処理系依存を回避するための指針
- 特定のランタイムやライブラリへの依存を最小限に抑える。
- クロスプラットフォームなライブラリや標準機能を活用する。
- テスト環境を多様化し、異なる環境での動作を検証する。
これらを意識することで、処理系依存による問題を防ぎ、堅牢なコードを実現できます。
処理系依存とは
以下のような特定の要因に依存したコードにより、異なる環境で動作が保証されない状態を指します。
- 非同期処理の依存：特定のイベントループやライブラリに依存。
- コンパイラやランタイムの差異：実装間で処理順序や動作が異なる。
- OSやハードウェア依存：特定のOSやハードウェアのみで動作するコード。
非同期処理の依存
非同期処理の実装が特定のランタイム（例: Pythonではuvloop）やイベントループに依存している場合、異なる環境で正常に動作しない可能性があります。
# ❌ uvloopに依存するコード
import asyncio
import uvloop
asyncio.set_event_loop_policy(uvloop.EventLoopPolicy()) # uvloopの利用を明示
async def fetch_data():
await asyncio.sleep(1)
print("Fetched data using uvloop")
asyncio.run(fetch_data())
# ⭕ ランタイムに依存しないコード
import asyncio
async def fetch_data():
await asyncio.sleep(1)
print("Fetched data in a portable way")
asyncio.run(fetch_data())
コンパイラやランタイムの差異
PythonのGIL（Global Interpreter Lock）の影響により、スレッドを用いた並列処理が他の処理系（例: Java）と異なる挙動を示す場合があります。
# ❌ スレッドによる並列処理 (PythonのGILに依存)
import threading
def print_numbers():
for i in range(5):
print(i)
threads = [threading.Thread(target=print_numbers) for _ in range(2)]
for t in threads:
t.start()
（Pythonでは、マルチプロセスを利用することで並列実行を実現し、GIL依存を回避します。）
# ⭕️ マルチプロセスを利用した並列処理
import multiprocessing
def print_numbers():
for i in range(5):
print(i)
if __name__ == '__main__':
processes = [multiprocessing.Process(target=print_numbers) for _ in range(2)]
for p in processes:
p.start()
OSやハードウェア依存
ファイルパスやシステムコールが特定のOSに依存している場合、他のプラットフォームで動作しない可能性があります。
# ❌ Windows専用のコード
file_path = "C:\\Users\\User\\file.txt"
クロスプラットフォームなライブラリ（例: osモジュール）を使用して、環境に依存しないコードを記述します。
# ⭕️ OSに依存しないコード
import os
file_path = os.path.join("Users", "User", "file.txt")
4.最適化とパフォーマンス
| 項目番号 | チェックポイント |
|---|---|
| 4-1 | 不要なループや計算処理を削除しているか |
| 4-2 | メモリの使用効率を考慮できているか |
| 4-3 | 要素の可変性を考慮しているか |
| 4-4 | 言語特有のベストプラクティスを理解できているか |
| 4-5 | キューやスタックの利用は適切か |
| 4-6 | N+1問題が起きていないか |
| 4-7 | 計算量を考慮できているか |
4-1.不要なループや計算処理を削除しているか
実行処理時間の増加やメモリ消費の増大は、システム全体の動作を遅くする原因となります。このようなパフォーマンスの問題を防ぐため、コードの最適化を行いましょう。
最適化したループのメリット
-
システムの応答性向上
- 実行速度が改善され、ユーザーに快適な体験を提供できます。
-
リソース効率の向上
- メモリやCPU使用量を削減することで、システム全体の負荷が軽減されます。
-
スケーラビリティの確保
- リソースを効率的に使うことで、システムが増加する負荷に対応しやすくなります。
# ❌️ 同じリストに複数回ループ
numbers = [1, 2, 3, 4, 5]
squared = []
filtered = []
for number in numbers:
squared.append(number ** 2)
for square in squared:
if square > 10:
filtered.append(square)
# ⭕ ワンパスで平方とフィルタリングを実行
numbers = [1, 2, 3, 4, 5]
filtered = [number ** 2 for number in numbers if number ** 2 > 10]
# ❌️ 毎回計算を繰り返す
numbers = [1, 2, 3, 4, 5, 4]
result = []
for number in numbers:
if numbers.count(number) > 1:
result.append(number)
# ⭕️ 事前にキャッシュした結果を使用
from collections import Counter
numbers = [1, 2, 3, 4, 5, 4]
counts = Counter(numbers)
result = [number for number in numbers if counts[number] > 1]
4-2.メモリの使用効率を考慮できているか
メモリ不足は、システム全体の動作を遅くし、パフォーマンスを著しく低下させる原因となります。
各プログラミング言語で利用可能なメモリ管理の特性を理解し、メモリ効率を最適化することが重要です。
（C言語であればメモリを手動で管理し、確保したメモリを解放できます。）
（Pythonではジェネレータ関数を使用し、メモリの使用を逐次的に利用できます。）
# ❌️ 1億個の数値をリストで保持して処理する
import time
import psutil
import os
# メモリ使用量を取得する関数
def get_memory_usage():
process = psutil.Process(os.getpid())
return process.memory_info().rss # メモリ使用量 (bytes)
# メイン処理
# 実行前の計測
start_time = time.time()
start_memory = get_memory_usage()
try:
numbers = [x ** 2 for x in range(100_000_000)] # リスト内包表記
total = sum(numbers)
# 実行後の計測
end_time = time.time()
end_memory = get_memory_usage()
# 結果の出力
print("合計:", total)
print("処理時間: {:.8f} 秒".format(end_time - start_time))
print("使用メモリ: {} バイト".format(end_memory - start_memory))
except MemoryError:
print("メモリ不足により処理が中断されました！")
# 出力結果
合計: 333333328333333350000000
処理時間: 6.75759292 秒
使用メモリ: 4002217984 バイト
# ⭕️ ジェネレータ式を使う
import time
import psutil
import os
# メモリ使用量を取得する関数
def get_memory_usage():
process = psutil.Process(os.getpid())
return process.memory_info().rss # メモリ使用量 (bytes)
# メイン処理
numbers = (x ** 2 for x in range(100_000_000)) # ジェネレータ式
# 実行前の計測
start_time = time.time()
start_memory = get_memory_usage()
# 合計を計算
total = sum(numbers)
# 実行後の計測
end_time = time.time()
end_memory = get_memory_usage()
# 結果の出力
print("合計:", total)
print("処理時間: {:.8f} 秒".format(end_time - start_time))
print("使用メモリ: {} バイト".format(end_memory - start_memory))
合計: 333333328333333350000000
処理時間: 6.54499197 秒
使用メモリ: 32768 バイト
処理時間は同じ6秒台ですが、使用メモリが圧倒的に違うことがわかりますね。
参考資料：
上級プログラマーのメモリ使用量を意識したコーディングとは？
パフォーマンスを意識したコーディングのガイドライン
4-3.要素の可変性を考慮しているか
プログラムでデータを管理する際、扱うデータが頻繁に変更されるか、それとも固定されたまま変更されないかを考えることは非常に重要です。この違いを理解し、適切なデータ型を選択することで、コードの効率や可読性が向上します。
まずは、リストとタプルの主要な特性を以下の表で確認してください。
| 特性/用途 | リスト | タプル |
|---|---|---|
| 可変性 | 可変 (Mutable) | 不変 (Immutable) |
| 変更操作 | 要素の追加・削除・変更が可能 | 要素の追加・削除・変更は不可 |
| 用途 | 頻繁に変更されるデータの管理 | 変更されないデータの管理 |
| メモリ効率 | メモリ消費が多い場合がある | メモリ消費が少なく、処理速度が速い場合がある |
| ハッシュ可能性 | リストはハッシュ可能ではない (キーとして使用不可) | タプルはハッシュ可能 (辞書のキーや集合の要素として使用可能) |
| 柔軟性 | 要素数や内容が変わる場合に適している | データが固定で変更の必要がない場合に適している |
Pythonには、要素の可変性に基づいて異なる特性を持つコレクション型として
リスト（List）とタプル（Tuple）があります。このセクションでは、リストとタプルの特徴を比較し、それぞれがどのような用途に適しているかを見ていきます。
リストの例：変更可能なデータに適している
shopping_list = ["apple", "banana", "orange"]
shopping_list.append("grape") # 要素を追加
print(shopping_list) # ['apple', 'banana', 'orange', 'grape']
タプルの例：変更されないデータに適している
DAYS_OF_WEEK = ("Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday", "Sunday")
print(DAYS_OF_WEEK[0]) # "Monday"
タプルの例：タプルをキーに使う例（ハッシュ可能）
location_data = {(10.0, 20.0): "Park"}
print(location_data[(10.0, 20.0)]) # "Park"
4-4.言語特有のベストプラクティスを理解できているか
言語には特有の機能があります。その言語毎の機能をよく理解して活用しましょう。
(本記事においてはPythonにおける、辞書とセットの使い分けについて例示します。)
辞書：商品の在庫管理
商品名をキー、在庫数を値として管理する場合に適しています。
inventory = {"apple": 10, "banana": 5, "orange": 8}
inventory["apple"] += 2 # 在庫の更新
print(inventory)
セット：参加者リストの重複排除
イベント参加者のリストから、重複を除いて一意なデータを作成します。
# 重複のないリストを作成
participants = ["Alice", "Bob", "Alice", "Charlie"]
unique_participants = set(participants)
print(unique_participants)
| 辞書 | セット |
|---|---|
| キーと値のペアでデータを格納 | 一意な要素を格納 |
| データのマッピング、頻度計算 | 重複排除、集合演算 |
| ユニークなキーが必要 | ユニークな要素のみ |
| 高速 (キーによるアクセス: 平均 O(1)) | 高速 (要素の存在確認: 平均 O(1)) |
| 商品名と価格の対応、カウント集計 | 重複を排除したリスト作成、2つのリストの共通要素を計算 |
4-5.キューやスタックの利用は適切か
データ構造のキューとスタックは、それぞれ異なる特徴と用途を持つ基本的なデータ構造です。どちらを選択するかは、要件に応じて適切に判断する必要があります。
キュー (Queue)
from collections import deque
# キューの作成
queue = deque()
# 要素の追加
queue.append("A")
queue.append("B")
# 要素の取り出し
print(queue.popleft()) # A
print(queue.popleft()) # B
スタック (stack)
# スタックの作成
stack = []
# 要素の追加
stack.append("A")
stack.append("B")
# 要素の取り出し
print(stack.pop()) # B
print(stack.pop()) # A
| 特性 | キュー | スタック |
|---|---|---|
| 操作 | FIFO（先入れ先出し) | LIFO（後入れ先出し) |
| 主な用途 | 順序処理が必要なデータ | 後戻り処理が必要なデータ |
| データ構造 | 一方向に進む | 逆順処理や一時的な保存が必要な場合に適する |
| 適切な利用場面 | タスクスケジューリング、データストリーム処理 | 再帰処理、逆順処理、アンドゥ機能 |
| 典型的な操作 | enqueue, dequeue | push, pop |
4-6.N+1問題が起きていないか
N+1問題は、データベースにおいて1回のクエリで済む処理を不適切な実装により余計なSQLクエリが重複して発行される問題を指します。これにより、システムのパフォーマンスが大幅に低下します。
# ❌️ N回クエリ実行
# 例: ORM（Django）を使用した場合
posts = Post.objects.all()
for post in posts:
comments = Comment.objects.filter(post=post)
print(f"Post: {post.title}, Comments: {len(comments)}")
# ⭕️ コメントを一括取得
posts = Post.objects.prefetch_related('comments').all()
for post in posts:
print(f"Post: {post.title}, Comments: {len(post.comments.all())}")
参考資料：
[宇宙一わかりやすい] n + 1 問題とはなにが問題なのか
N+1問題とその解決方法
4-7.計算量を考慮できているか
扱うデータ量が少ない場合はそれほど考慮する必要はありませんが、データ量の桁が増えてくると、計算量を考慮する必要が出てきます。
具体的な例を元に、計算ロジックによってどれくらい実行時間に差が出るのか見てみましょう。
今回はfor文とPandas DataFrameを操作する際のコードの効率性について、実行時間に焦点を当てた結果になります。プログラム初心者がよく使うfor文の方法と、Pythonicな方法であるmapメソッドを比較し、どちらがより効率的で実用的かを明らかにします。
コード例の処理概要
①実験用データとして、26列、n行のデータフレームを作ります。「n行」を「1,000」「5,000」「10,000」「50,000」としてそれぞれデータフレームを作る。(generate_data)
②作成したデータフレームに対して、A列の数字が、
5より少ないとLOW
5から8だとMedium
8以上だとHigh
を新しく作成するResult列に値を入れるプログラムになっています。（method1, method2）
import pandas as pd
import numpy as np
import time
# 定数
LOWER_SPLIT = 5
UPPER_SPLIT = 8
AGE_LABEL_LOW = "Low"
AGE_LABEL_MEDIUM = "Medium"
AGE_LABEL_HIGH = "High"
ALPHABET = list("ABCDEFGHIJKLMNOPQRSTUVWXYZ") # A～Z
# データフレームの生成
def generate_data(n_rows, file_name):
data = pd.DataFrame(
np.random.uniform(0, 10, size=(n_rows, len(ALPHABET))),
columns=ALPHABET
)
# ファイル保存（インデックスなし）
data.to_csv(file_name, index=False)
return data
# ❌️ 方法1: for文で処理
def method1(data):
_data = data.copy()
_data.loc[:, 'Result'] = None
for i in range(len(_data)):
value = _data.iloc[i, 0] # A列の値を使用
if value < LOWER_SPLIT:
_data.iloc[i, -1] = AGE_LABEL_LOW
elif LOWER_SPLIT <= value < UPPER_SPLIT:
_data.iloc[i, -1] = AGE_LABEL_MEDIUM
else:
_data.iloc[i, -1] = AGE_LABEL_HIGH
return _data
# ⭕️ 方法2: mapメソッドで処理
def method2(data):
def categorize(value):
if value < LOWER_SPLIT:
return AGE_LABEL_LOW
elif LOWER_SPLIT <= value < UPPER_SPLIT:
return AGE_LABEL_MEDIUM
else:
return AGE_LABEL_HIGH
_data = data.copy()
_data.loc[:, 'Result'] = _data['A'].map(categorize) # A列の値を使用
return _data
# 実行時間の測定
def measure_time(method, data):
start_time = time.time()
method(data)
end_time = time.time()
return end_time - start_time
# メイン処理
if __name__ == "__main__":
sample_sizes = [1000, 5000, 10000, 50000]
times1, times2 = [], []
for size in sample_sizes:
print(f"\n{size}行のデータを処理しています...")
# データ生成
data = generate_data(size, f"data_{size}.csv")
# 方法1
time1 = measure_time(method1, data)
times1.append(time1)
# 方法2
time2 = measure_time(method2, data)
times2.append(time2)
print(f"方法1 - 実行時間: {time1:.4f}秒")
print(f"方法2 - 実行時間: {time2:.4f}秒")
for文とmapメソッドの実行時間を比較してみましょう。
グラフにすると一目瞭然ですね。
| 行数 |
for文の実行時間 |
mapメソッドの実行時間 |
|---|---|---|
| 1000 | 0.3375 | 0.0019 |
| 5000 | 3.2473 | 0.0042 |
| 10000 | 4.4141 | 0.0083 |
| 50000 | 10.1970 | 0.0254 |
5.テストの充実とPR作成に関して
| 項目番号 | チェックポイント |
|---|---|
| 5-1 | 今回の修正と関係ない修正が紛れていないか |
| 5-2 | PRの説明欄に今回の修正内容が簡潔に記載されているか |
| 5-3 | レビューチェック観点が分かりやすいPRになっているか |
| 5-4 | 動作確認項目が必要十分に網羅されているか |
| 5-5 | テストコードは必要十分に網羅しているか |
5-1.今回の修正と関係ない修正が紛れていないか
不必要な変更や関係のない修正がcommitに含まれると、以下のような問題が発生する可能性があります。
不必要なcommitが増えると
-
履歴が汚れる
- 不要な変更が混ざると、修正内容の追跡が難しくなります。
-
他メンバーの作業が上書きされる
- 自分が意図していない箇所を間違って変更すると、チームに影響を与えます。
git diff # ⭕️ コマンドで変更内容を確認
git diff --staged # ⭕️ ステージングエリアの変更を確認
5-2.PRの説明欄に今回の修正内容が簡潔に記載されているか
❌️ 修正しました。確認お願いします。
### ⭕️ 修正内容
- 入力フォームのバリデーションエラー表示を改善
### 修正の背景
- 一部のケースでバリデーションエラーが正しく表示されない問題が発生していました。
### 詳細
1. バリデーションロジックをリファクタリング
2. エラー表示用の関数を新規作成
3. 単体テストを追加し、既存のテストケースを修正
### 動作確認手順
- 正常なメールアドレスを入力 → 送信成功メッセージが表示される
- 空欄で送信 → 「メールアドレスを入力してください」というエラーが表示される
### 関連するタスク
- Issue: #123
### レビュー観点
- バリデーションロジックが既存コードに影響を与えていないか確認してください。
- 単体テストが十分かどうか確認してください。
5-3.レビューチェック観点が分かりやすいPRになっているか
スコープを分けたPRを作成する
1つのPRに複数の修正内容を詰め込むと、レビュアーの負担が増え、修正内容を正確に把握しづらくなります。
修正のスコープを分け、1つのPRでは1つの目的に絞ることで、レビューを効率的に進められます。
❌️ Before
バリデーションエラーの修正とスタイル修正を1つのPRに含める。
PRの説明
## 修正内容:
- 入力時のバリデーションエラーを修正
- テストケースを追加
- ファイル全体のインデント修正
- コメントの整理
⭕️ After
バリデーションエラーの修正とスタイル修正を別々のPRに分ける。
### PR: バリデーションエラーの修正
## 修正内容:
- 入力時のバリデーションエラーを修正
- テストケースを追加
⭕️ After
### PR: スタイル修正
## 修正内容:
- ファイル全体のインデント修正
- コメントの整理
PRには具体的な説明とコード例を記載する
PRには修正内容の背景、目的、および具体的な修正内容を含めることで、レビュアーが理解しやすくなります。可能であれば、コードのBefore/Afterも示すと良いです。
❌️ Before:
## 修正内容
- バリデーションエラーを修正
- インデントを修正
⭕️ After:
markdown
## 修正内容
### 修正前
```python
# 修正前: バリデーションが適切に機能していない
def validate_name(name):
if len(name) == 0:
return False
###修正後
python
### 修正後: バリデーション条件を追加
def validate_name(name):
if not name or len(name.strip()) == 0:
return False
### 修正の背景
ユーザー入力が空白のみの場合にバリデーションを通過してしまう問題が発生していました。
5-4.動作確認項目が必要十分に網羅されているか
クライアントの要件に応えるためには、要件の背景を深く理解し、システム全体への影響を適切に評価する必要があります。
直接的な影響の視点
-
変更の影響範囲を確認
- 変更した箇所が他のモジュールや機能から参照されている場合、その依存関係に悪影響がないかを確認します。
- 特に、頻繁に呼び出される関数やクラスでは、小さな変更がシステム全体に波及する可能性があります。
-
クライアントの期待する挙動
- 要件の意図を正確に理解し、クライアントが想定するシナリオで期待どおりに動作するかを検証します。
- 例えば、「エラーメッセージの表示」「入力データのフォーマット」「処理速度」など、クライアントが重視するポイントに重点を置きます。
間接的な影響の視点
-
データ構造の変更
- データベーススキーマの変更が既存のデータや関連機能に影響を及ぼさないかを確認します。
- 既存データが新しいスキーマに適合しているかを検証し、必要に応じてデータマイグレーションを計画します
-
既存機能の動作確認
- 変更が直接関係のない既存の機能や画面にも影響を与えていないかを確認します。
- 特に、ユニットテストや結合テストを用いて、意図しない副作用を発見します。
-
外部システムとの連携
- 外部APIやサードパーティライブラリを使用している場合、それらへの影響を確認します。
- 例えば、外部APIの仕様変更や、ライブラリのアップデートが影響を及ぼさないかを検討します。
5-5.テストコードは必要十分に網羅しているか
コードカバレッジ（Code Coverage）は、テストで実行されたコードの割合を測定する指標です。
カバレッジを向上させることで、テストの網羅性が高まり、バグや問題の早期発見につながります。
コードカバレッジ向上の重要性
-
信頼性の向上
- カバレッジが高いほど、コードがテストで検証されている箇所が多くなり、予期しないバグや挙動のリスクを低減できます。
-
保守性の向上
- 十分にテストされたコードは、変更が加えられた際にその影響を検証しやすく、保守性が向上します。
-
複雑性の低下
- テストを通じて、コードが複雑すぎる箇所や冗長な箇所を見直すきっかけとなります。これにより、コードの可読性やメンテナンス性が向上します。
# テスト元の関数
def calculate_discount(price, is_member):
"""会員の場合10%割引する関数"""
if is_member:
return price * 0.9
return price
# ❌️ テストが局所的
def test_calculate_discount_bad():
"""テストコードが不十分な悪い例"""
assert calculate_discount(100, True) == 90 # 会員のケースのみテスト
# ⭕️ テストが網羅されている
def test_calculate_discount_good():
"""テストコードを網羅"""
# 正常系のテスト
assert calculate_discount(100, True) == 90 # 会員の場合
assert calculate_discount(100, False) == 100 # 非会員の場合
# 異常系のテスト
assert calculate_discount(0, True) == 0 # 価格が0の場合
assert calculate_discount(-100, True) == -90 # 負の値の場合
6.APIレビュー
| 項目番号 | チェックポイント |
|---|---|
| 6-1 | 他のAPIとの兼ね合いは適切か |
| 6-2 | 外部サービスのAPIを呼び出す際はエラー処理をしているか |
| 6-3 | リクエストパラメータの存在確認とデータ検証はしているか |
| 6-4 | トランザクションの処理はデッドロックが発生しないか |
| 6-5 | APIテストが書かれているか |
6-1.他のAPIとの兼ね合いは適切か
コードの可読性や保守性を高めるために、命名規則の一貫性と粒度（階層）の明確さを適切に設計することが重要です。これらが不適切だと、以下のような問題が発生する可能性があります。
APIの一貫性について
-
コードの可読性が低下
- 命名に一貫性がないと、意図がわかりにくくなり、コードを理解するのに時間がかかります。
-
保守性が低下
- 命名が曖昧だったり粒度が不明確だと、コード変更時のミスやバグの原因になります。
-
チーム間の認識ズレ
- チーム全体で統一されていないと、コラボレーションが難しくなります。
# ❌️ 各エンドポイントの名前がバラバラに成っている
GET /tasks
POST /task
PUT /{task_id} # 適切なルートの指定をされていない。
# ⭕️ ユニークなAPIパス（/user/tasks）で統一
GET /tasks
POST /tasks
PUT /tasks/{task_id} # ルート設計が明確になり、コードの可読性が向上
6-2.外部サービスのAPIを呼び出す際はエラー処理をしているか
外部サービスのAPIを呼び出す際は、次のポイントを考慮し、適切なエラー処理を行う必要があります。
外部ALPIサービスを呼び出すときのポイント
-
タイムアウトの設定
- 外部サービスの応答が遅れると、システム全体のパフォーマンスに影響を及ぼします。タイムアウトを設定し、遅延の影響を最小化しましょう。
-
エラー内容のログ出力
- エラーが発生した際に詳細なログを記録することで、原因の特定や監視が容易になります。
-
リトライ処理
- 一時的なエラー（ネットワーク障害など）に対しては、リトライを実施することでエラーからの復旧を図ります。
# ❌️ エラー時に適切なログやリトライ処理がない
import requests
def fetch_data():
response = requests.get("https://api.example.com/data")
return response.json()
# エラー処理がなく、外部APIが失敗した場合（例: タイムアウト、404エラーなど）
# にプログラムがクラッシュする
# ⭕️ エラー時に適切なログやリトライ処理ができている
import requests
def fetch_data():
try:
response = requests.get("https://api.example.com/data", timeout=5)
response.raise_for_status() # HTTPエラーを例外としてスロー
return response.json()
except requests.exceptions.HTTPError as http_err:
print(f"HTTP error occurred: {http_err}") # エラーログを出力
except requests.exceptions.RequestException as req_err:
print(f"Request error occurred: {req_err}") # その他のリクエストエラーを出力
return None # エラー時はNoneを返す
6-3.リクエストパラメータの存在確認とデータ検証はしているか
外部から受け取るリクエストパラメータに対して、以下を確認することが重要です。
リクエストパラメータの確認ポイント
-
必須パラメータの確認
- 必須パラメータが不足している場合にエラーを発生させ、適切なレスポンスを返します。
-
データ型の検証
- 不正なデータ型や形式のパラメータが入力された場合、そのまま処理されないようにします。
-
セキュリティリスクの最小化
- 入力データを検証し、不正な値（SQLインジェクションやスクリプト）を防ぎます。
# ❌️ 存在チェックもデータ型の検証もなし
from flask import Flask, request, jsonify
app = Flask(__name__)
@app.route('/api/process', methods=['POST'])
def process_data():
param = request.json.get('param')
result = f"Processed: {param}"
return jsonify({"result": result})
# ⭕️ 存在チェックもデータ型の検証もできている
from flask import Flask, request, jsonify
from functools import wraps
app = Flask(__name__)
# カスタム例外クラス
class ValidationError(Exception):
def __init__(self, message):
self.message = message
super().__init__(message)
# バリデーション処理
def validate_param(param):
if param is None:
raise ValidationError("Missing 'param'.")
if not isinstance(param, str):
raise ValidationError("'param' must be a string.")
if not param.strip():
raise ValidationError("'param' must not be empty.")
# デコレータでバリデーションを適用
def validate_request_params(validation_function):
def decorator(func):
@wraps(func)
def wrapper(*args, **kwargs):
data = request.json
try:
param = data.get('param') # リクエストパラメータを取得
validation_function(param) # バリデーションを実行
except ValidationError as e:
return jsonify({"error": e.message}), 400 # バリデーションエラーを返す
return func(*args, **kwargs)
return wrapper
return decorator
@app.route('/api/process', methods=['POST'])
@validate_request_params(validate_param)
def process_data():
data = request.json
param = data['param']
result = f"Processed: {param}"
return jsonify({"result": result})
6-4.トランザクションの処理はデッドロックが発生しないか
ある一連の処理をまとめた単位をトランザクションといい。
ACID特性というデータベースの安定性と整合性を確保するための基本的な要件があり、この要件が満たさなければ確定（コミット）されないです。
例えばATMで、お金はおろしていないのに端末操作は完了しているということで、残高が減っていたら嫌ですよね。
トランザクション管理のポイント
-
明示的にトランザクションを開始
- トランザクションを明示的に開始し、コミットやロールバックを適切に制御します。
-
エラー時のロールバックでデータ整合性を確保
- トランザクション中にエラーが発生した場合、ロールバックを実行して中途半端な状態を防ぎます。
-
ロックの範囲を最小化
- トランザクションを適切に分割し、長時間ロックを保持しないようにして、競合を回避します。
-
エラー内容をログに記録
- 発生したエラーの詳細をログとして記録し、問題の原因を追跡しやすくします。
# ❌️ デッドロックが生じる
import sqlite3
def process_transactions():
conn = sqlite3.connect('example.db')
cursor = conn.cursor()
# トランザクション処理1
cursor.execute("BEGIN")
cursor.execute("UPDATE accounts SET balance = balance - 100 WHERE id = 1")
cursor.execute("UPDATE accounts SET balance = balance + 100 WHERE id = 2")
# トランザクション処理2
cursor.execute("UPDATE accounts SET balance = balance + 50 WHERE id = 3")
conn.commit()
conn.close()
# ⭕️ トランザクション処理が出来ている
import sqlite3
def process_transactions():
conn = sqlite3.connect('example.db')
cursor = conn.cursor()
try:
# トランザクション処理1
conn.execute("BEGIN IMMEDIATE") # トランザクションを明示的に開始
cursor.execute("UPDATE accounts SET balance = balance - 100 WHERE id = 1")
cursor.execute("UPDATE accounts SET balance = balance + 100 WHERE id = 2")
# トランザクション処理2
cursor.execute("UPDATE accounts SET balance = balance + 50 WHERE id = 3")
conn.commit() # トランザクションを正常に終了
except sqlite3.DatabaseError as e:
conn.rollback() # エラー時にロールバック
print(f"Transaction failed: {e}")
finally:
conn.close()
ロールバックとは
トランザクションがエラーや不具合で正常に終了できなかった場合に、トランザクション開始前の状態に戻す操作を指します。
これにより、データの不整合や破損を防ぎ、システム全体の整合性を保つことができます。
デッドロックとは
複数のトランザクションが相互にリソースを待ち合ってしまうことです。
例えば
- トランザクションAがリソースXをロックする。
- トランザクションBがリソースYをロックする。
- AがYを要求するが、Bがロックしているため待機状態になる。
- BがXを要求するが、Aがロックしているため待機状態になる。
- これにより、AとBがお互いにリソースを待ち続け、進行不能な状態に陥る。
トランザクションとは | 「分かりそう」で「分からない」でも「分かった」気になれるIT用語辞典
トランザクション処理とは？メリット・デメリットや活用例、バッチ処理との違いも解説
デッドロック 【deadlock】
排他制御
6-5.APIテストが書かれているか
APIテストは、APIの動作確認や品質保証を目的とした重要なプロセスです。APIテストがないと、以下の問題が発生する可能性があります。
APIテストがされていない時のデメリット
-
意図しない不具合を検知できない
- APIの変更やリファクタリングの際、動作が壊れても気づかず、システム全体に影響を及ぼす可能性があります。
-
一貫性のある確認が難しい
- 手動確認に依存すると、確認の抜け漏れやバラツキが発生します。
-
バグ発生時の原因特定が困難
- 自動化されたAPIテストがない場合、バグの原因を特定するまでの時間が長くなります。
-
APIの信頼性が低下
- 適切なテストが行われていないと、クライアントや他のサービスとの連携時にエラーが発生する可能性が高まります。
改善例
# ⭕️ テストが出来ている
import unittest
from app import app # Flaskアプリケーションをインポート
class TestParam(unittest.TestCase):
"""
Flaskアプリケーションのテストクラス。
"""
def setUp(self):
"""
テストの前に実行されるセットアップ。
Flaskのテストクライアントを設定。
"""
self.app = app.test_client()
self.app.testing = True
def test_missing_param(self):
"""
必須パラメータが欠けている場合のテスト。
正しくエラーを返すかを確認。
"""
response = self.app.post(
'/api/process', # テスト対象のエンドポイント
json={} # パラメータを送らない
)
# レスポンスが 400 Bad Request であることを確認
self.assertEqual(response.status_code, 400)
# レスポンスに "error" キーが含まれていることを確認
self.assertIn("error", response.get_json())
# エラーメッセージが "Missing or invalid 'param'" であることを確認
self.assertEqual(response.get_json()["error"], "Missing or invalid 'param'")
if __name__ == "__main__":
unittest.main()
7.まとめ
いかがだったでしょうか？
この記事では、コードレビューのさまざまなポイントについて取り上げました。
ご自身のコードを振り返る際の参考や、後輩への指導を行う際の礎となることを願っています。
また、この記事に誤りや改善点がある場合は、ぜひご指摘いただけますと幸いです。皆さまからのフィードバックを基に、内容をより良いものへと更新していきたいと思います。
最後までご覧いただき、ありがとうございました！
参考資料
知らないと恥ずかしいコードレビューで指摘されがちなポイント14選
コードレビュー虎の巻
弊社Nucoでは、他にも様々なお役立ち記事を公開しています。よかったら、Organizationのページも覗いてみてください。
また、Nucoでは一緒に働く仲間も募集しています！興味をお持ちいただける方は、こちらまで。