コードレビュー面接の練習：不具合の優先順位と伝わるコメント
Quick Overview
在庫取り置きの小さなPRを題材に、部分更新と競合を再現。修正を必須とする根拠、任意の改善、伝わる日本語コメントを整理します。
「変数名が短い」とコメントを書き始めたあとで、失敗したはずの注文が在庫を減らしていると気づく。コードレビュー面接では、先に直すべき問題を、根拠とともに説明する練習が役立ちます。まず小さな入力で状態の変化を追い、影響と修正の確認方法をセットにして伝えましょう。
この記事では、在庫をまとめて取り置きする短い Python の PR を読みます。失敗時の部分更新と、二つの要求が同じ在庫を読んだ場合の不整合を別々に再現し、日本語のレビューコメントに落とし込みます。コードと結果は PracHub のオリジナル練習で、実在する企業の出題や倉庫の障害記録ではありません。
公式に文書化された事実は Google のレビュー指針と Python 3.12 の仕様に帰属させます。観測結果は CPython 3.12.14 で実行したローカルの例、優先順位・発言例は編集部の提案です。候補者の体験談は使用しておらず、この形式の採用頻度や合格基準は断定しません。コメントの議論は Resolve a Disagreement in Code Review でも練習できます。
コメントを書く前に、守るべき条件を確認する
今回の契約は「要求した全商品を取り置きできる場合だけ成功し、拒否した場合は在庫を一切変えない」です。在庫は SKU ごとの非負整数、要求数は正の整数とします。未知の SKU と在庫不足は False、形や型が不正な入力は ValueError で拒否します。成功時は True を返します。
初期状態は A が5個、Bが1個。要求は {"A": 2, "B": 2} です。B が不足するため、結果は拒否、在庫は A が5個、Bが1個のままでなければなりません。ここを先に言えば、「途中まで確保してよい仕様では？」という認識のずれを避けられます。
並行要求も対象なら、成功した要求数の合計が利用可能数を超えない条件が加わります。一方、今回の関数は一つのプロセス内のメモリを扱います。永続化、複数サーバー、予約の有効期限、認証、返品による補充は別の設計課題です。呼び出し中に別スレッドが入力辞書そのものを書き換える利用も、契約に含めません。
公式指針： Google のレビューで確認する観点は、機能、設計、複雑さ、テストなどを対象にしています。Google の開発指針を、応募先の面接採点表と同一視しないことも大切です。本稿では、この契約に対する違反を優先して調べます。
小さな PR を読み、失敗した経路を追う
次のコードは意図的に欠陥を含みます。after_read は再現テスト用のフックで、通常の呼び出しでは何もしません。入力の検証と共有状態の所有権が不足している点も、レビュー対象です。
def reserve_bad(stock, request, after_read=lambda: None):
candidate = stock.copy()
for sku, quantity in request.items():
available = candidate[sku]["available"]
if quantity > available:
return False
after_read()
candidate[sku]["available"] = available - quantity
stock.update(candidate)
return True
A を2個減らした直後に B の不足が分かり、関数は False を返します。しかしローカルで観測した在庫は A が3個、Bが1個でした。最後の stock.update(candidate) に到達していなくても、A の変更が残っています。
公式仕様： Python の浅いコピーと深いコピーでは、浅いコピーは外側のコンテナを新しくしても、その中のオブジェクトへの参照を保持します。この例では stock["A"] と candidate["A"] が同じ内側の辞書を指すため、available の代入が元の在庫にも見えます。
「コピーしているので安全です」という作者の説明には、外側と内側のどちらをコピーしたかで答えます。紙に二つの外側の箱と一つの A の箱を描き、両方の矢印を A に向けると説明できます。単に return False を別の場所へ移すだけでは、変更した後の巻き戻しを保証できません。
なお、深いコピーへ置き換えるだけで並行実行の問題まで解決するわけではありません。失敗時に共有状態を汚さないことと、複数要求の確認・更新を競合させないことは、別々の条件です。修正案もテストも、この二つを分けて考えます。
最終在庫が正でも、二件成功は誤りになり得る
次に初期在庫を A の5個だけにし、二つのスレッドがそれぞれ4個を要求します。欠陥版では読み取り直後のフックに Barrier(2) を置き、両方が5を読み終えてから先へ進めました。待ち時間を長くして偶然の競合を期待する再現ではありません。
| 制御した順序 | 要求1 | 要求2 |
|---|---|---|
| 読み取り | 利用可能数5を読む | 利用可能数5を読む |
| ローカルの計算 | 5 − 4 = 1 | 5 − 4 = 1 |
| 書き込みと返却 | 1を書き、成功 | 1を書き、成功 |
| 観測結果 | 成功した要求は合計2件 | 受付数量は合計8個、最終在庫は1個 |
在庫が負でないことだけを確認するテストは、この不整合を見逃します。二つとも同じ値を上書きするため、残数1という見た目は一件だけ成功した場合と同じです。成功応答と在庫を照合すれば、残数が正でも受付合計が在庫を超える不整合を見つけられます。
公式仕様： Python の threadingはロックやバリアの同期動作を説明しています。GIL があるから複数の読み取り・判定・書き込みをまとめた業務操作まで不可分になる、とは言えません。本例は読み取りの間に同期点を置き、成立する実行順序を明示しています。
推論： この結果から、確認と更新に同じ同期の境界が必要だと判断できます。ただし、二件の再現から本番での発生率や負荷耐性は求められません。スレッドの勝者も仕様にしていません。実行した修正版のテストは勝者を固定せず、返答が一件成功・一件拒否、最終在庫が1個であることを確認しました。
指摘を、影響と確信度で並べ替える
この練習では、次の順序を提案します。表の「必須」は、全件成功か無変更で拒否する契約と、同時呼び出しを許す条件に基づく編集上の判断です。応募先の P0/P1 の定義や実際の障害優先度を推測したものではありません。
| 指摘 | 再現できる影響 | 提案する扱い | 修正確認の条件 |
|---|---|---|---|
| 拒否後に A だけ減る | 取り置き不成立なのに在庫が減少 | 必須 | A2・B2 を拒否して全状態が初期値と一致 |
| 二要求が両方成功する | 在庫5に対し合計8を受付 | 必須。並行呼び出しが契約に含まれる場合 | 成功と拒否が一件ずつ、残数1 |
| 負数を受け入れる | A に −2 を要求すると在庫が7へ増加 | 必須。入力がこの境界で未検証の場合 | 負数・真偽値・小数を変更前に拒否 |
candidate の役割が曖昧 | コピーと確定の境界を読み違えやすい | 動作修正後の改善候補 | 独立した候補状態であることが実装と一致 |
| 好みだけの改名や整形 | 現時点で契約違反を示せない | 任意 | チームの規約に沿う場合に提案 |
優先順位は、影響の大きさだけで機械的に決めません。入力が上流で確実に検証されるなら、負数の問題の責任箇所は変わります。反対に、並行実行がないと確認できれば競合の指摘は現行契約の必須修正から外れます。条件を確認してから分類し直す姿勢を示しましょう。
「危険そう」「遅そう」という感想は、再現済みの指摘と区別します。性能については、深いコピーの対象サイズや呼び出し頻度を確認し、計測が必要な仮説として残せます。未計測の改善案を、すでに確認した在庫の不整合と同じ確信度で書かないことが重要です。
作者が動けるレビューコメントにする
公式指針： Google のコメントの書き方は、コードに焦点を当てること、理由を説明すること、必須と任意の指摘を明確にすることを勧めています。丁寧な疑問形にするだけでは、何を直すかは伝わりません。条件・観測・影響・依頼の順に具体化します。
必須のコメント例：「全商品を確保できない場合は変更しない契約なら、ここは修正をお願いします。A=5、B=1 に A2・B2 を渡すと False ですが A が3に変わります。浅いコピーで内側の辞書を共有しているためです。候補状態を元と分離し、拒否時の全状態一致を確認するテストを追加できますか。」
並行実行についての例：「この関数は同時に呼ばれますか。同時実行を許す場合、二件が A=5 を読んで4個ずつ成功し、合計8個を受け付ける順序を再現できました。確認から確定まで同じ同期境界に入れる修正と、一件だけ成功するテストが必要です。」
任意のコメント例：「任意：動作修正後、候補状態を作る箇所と確定する箇所が分かる名前にすると、この契約を追いやすくなりそうです。既存の命名規約があればそちらに合わせてください。」
同じ箇所に似たコメントを何個も残すより、一つの根本原因と関連する失敗例をまとめます。ただし部分更新と競合のように修正条件が違うものは、一件に押し込めません。「全体的にリファクタリングしてください」より、小さく確認できる依頼の方が議論を進めやすくなります。
修正案では、コピー・所有権・ロックの範囲を説明する
修正版では Inventory が初期在庫を深くコピーして所有し、読み取り用の snapshot() も独立したコピーを返します。外側から内側の辞書を直接書き換えられる状態では、メソッド内だけをロックしても守れません。入力を検証したあと、同じロック内で候補状態を作り、全 SKU を確認し、最後に確定します。
次は実行したクラスの reserve メソッドです。deepcopy と Lock はインポート済み、self._stock と self._lock は初期化済みという前提で読みます。
def reserve(self, request):
if type(request) is not dict or not request:
raise ValueError("nonempty request dict required")
items = tuple(request.items())
if any(type(sku) is not str or not sku
or type(qty) is not int or qty <= 0
for sku, qty in items):
raise ValueError("positive integer quantities required")
with self._lock:
candidate = deepcopy(self._stock)
for sku, quantity in items:
if (sku not in candidate
or quantity > candidate[sku]["available"]):
return False
candidate[sku]["available"] -= quantity
self._stock = candidate
return True
type(qty) is int は、この練習で真偽値も拒否するための意図的な条件です。入力値を暗黙に整数へ変換せず、契約に合わない型を拒否します。また、ロックを代入の前後だけに置いても、二件が古い残数を使って判定する窓は残ります。確認に使う状態と、その判定に基づく更新を同じ境界で保護します。
この方法は全在庫をコピーし、全要求を一つのロックで直列化するため、大きな在庫や高頻度の処理にはコストがあります。小さな練習で契約を明示する修正として選んでいます。実システムで行ロックや条件付き更新を使う場合も、複数商品の整合性、失敗時の取り消し、競合時の返答を改めて定義してください。
テストが証明した範囲を、件数と一緒に伝える
ローカルでは24個の unittest テストが通りました。欠陥版の三つの挙動を確認するテストも含みます。したがって「24件通過」は、元の PR が正しいという意味ではありません。欠陥の再現と修正版の回帰確認を、同じ実行記録に残したという意味です。
修正版では正常な複数商品、残数ちょうど、在庫不足、途中に未知の SKU がある拒否、負数・ゼロ・真偽値・小数・文字列・空要求などを確認しました。初期入力と返却スナップショットを書き換えても内部在庫が変わらないこと、要求入力を変更しないこと、拒否のあとに別要求が成功することも確認しています。
並行実行テストでは、欠陥版だけに読み取り後のバリアを入れています。修正版のロックの内側で二スレッドをバリア待ちにすると、片方がロックを持ったまま相手を待つため、適切なテストになりません。修正版は呼び出し前に二件を揃え、終了後に返答と残数を照合しました。
この確認は単一プロセスのメモリ上の動作です。プロセス停止時の耐久性、データベースの分離レベル、ネットワーク再試行、複数プロセス、負荷時の待ち時間も、この24テストの対象外です。面接でも「このテストで確認できたこと」と「追加の仕組みが必要なこと」を分けて説明します。
意見が割れたら、反例と判断の担当を明確にする
作者から「今のテストは全部通っています」と返された場合は、既存テストがどの順序を扱うかを確認します。「二件が同じ値を読んだケースは含まれていますか。こちらの再現では成功二件、残数1になります」と、追加したい条件を共有します。相手の能力を評価する言い方に変える必要はありません。
仕様が不明なら、確定事項と仮定を分けて保留します。「同時実行なしが運用上の保証なら、この競合を今回の必須修正から外せます。その保証の場所を確認したいです」という発言は、根拠に応じて判断を変える例です。単に意見が割れたから危険を受け入れる、とは説明しません。
合意できないリスクが残る場合は、再現手順、影響、選択肢を整理し、サービスの責任者や別のレビュアーへ判断を依頼する方針を伝えます。誰がどの前提で決めたかを記録し、決まった制約が次の変更で失われないようコードやテストへ反映します。これは練習上の提案で、特定企業のエスカレーション規程ではありません。
口頭で締めるなら、「全体として読みやすいか」だけで終わらず、必須修正二点、条件確認一点、任意改善一点のように整理します。それぞれに短い反例と確認方法を付ければ、作者が修正箇所と完了条件を確認しやすくなります。
五つの練習で、見つける力と伝える力を分けて確認する
次のリンクは PracHub で公開されている題名をそのまま示しています。今回の在庫 PR が、その企業で出題されたという意味ではありません。キャッシュや GIL の問題では共有状態の境界を、対立の問題では証拠の伝え方を重点的に練習できます。
| PracHub の問題 | 今回とのつながり | 回答後に確認すること |
|---|---|---|
| Resolve a Disagreement in Code Review | 意見の違いを反例で具体化する | 相手の主張と自分の確認結果を別々に説明したか |
| How do you conduct a code review exercise? | 契約、影響、優先順位を整理する | 指摘一覧が動作上の重要度で並んでいるか |
| Code Review: Thread Safety of a Python Compute-and-Cache Function | 共有状態の確認から更新までを追う | 在庫とは異なるキャッシュの契約を確認したか |
| Design scalable inventory system and avoid races | 単一プロセスの修正から設計へ広げる | 複数サーバーで同じロックが使えると思い込んでいないか |
| Discuss Python mutability, copying, and GIL | コピーと同期を別々に説明する | 外側のコピー、内側の参照、複合操作を区別したか |
次は Resolve a Disagreement in Code Review を開き、「条件・観測・影響・依頼」の四点で一つのコメントを書いてみてください。本稿の反例を別のコードへそのまま当てはめず、その問題の契約から優先順位を組み立て直すのが練習になります。
Sources and Further Reading
- Google Engineering Practices — What to look for in a code review：機能・設計・テストを含むレビュー観点。
- Google Engineering Practices — How to write code review comments：コメントの理由、表現、必須と任意の区別。
- Python 3.12 — copy：浅いコピーと深いコピーの参照関係。
- Python 3.12 — threading：ロックとバリアの同期動作。本稿の再現は CPython 3.12.14。
Comments (0)