概要
マジックナンバーの排除(Ch10)
注文ステータス 3・返金理由 1・閾値 0.5 という生の数値を OrderStatus/RefundReasonCode(StrEnum)と MAX_PARTIAL_REFUND_RATIO という名前付き定数に置き換え、コードだけで意味が分かる状態にする。
null問題(複数の意味を1値に圧縮しない)(Ch10)
「注文が存在しない」「返金対象外」「決済ゲートウェイ障害」という3つの異なる失敗が None/-1 の2種類に押し込められていたのを、OrderNotFoundError/RefundNotEligibleError/PaymentGatewayError という専用の例外クラスに分離する。
例外の握り潰し防止(Ch10)
except Exception: return None で決済ゲートウェイの障害を無言で消していたのを、raise PaymentGatewayError(...) from exc にして原因の例外(__cause__)を保持したまま専用例外に変換する。
YAGNI(Ch10)
「将来の複数返金方式に備えて」用意されていた register_strategy/strategies は一度も呼ばれていなかったため削除。必要になった時点で第8章のストラテジパターンを導入する方針をコメントに残すだけにとどめる。
問題
ECサイト MOps チームでは、返品承認後の返金処理を行う RefundHandler を Python で実装している。以下の「悪いコード」は、注文ステータス・返金理由を生の整数(3, 4, 1)で判定し、「注文が存在しない」「返金対象外」「決済ゲートウェイ障害」という3つの全く異なる失敗が None と -1 の2種類の戻り値に押し込められている。さらに決済ゲートウェイ呼び出しは except Exception: return None で無言のまま握り潰され、ログもスタックトレースも残らない。加えて、将来の複数返金方式に備えたつもりの register_strategy/strategies というプラグイン登録機構が用意されているが、実際に呼ばれる方式は1つしかない。
制約・前提条件
- Python 3.12+、値オブジェクトには
dataclass(frozen=True, slots=True)を使用すること - 注文ステータス・返金理由コードは
StrEnumで表現すること。DBが返す生の整数コードは境界(get_order直後)でのみ扱い、その先には漏らさないこと - 「注文なし」「返金対象外」「決済ゲートウェイ障害」は
None/-1に押し込めず、それぞれ専用の例外クラスで表現すること(OrderNotFoundError/RefundNotEligibleError/PaymentGatewayError) - 決済ゲートウェイの例外は握り潰さず、
raise ... from excで原因を保持したまま専用例外に変換すること - 半額超の返金を許容する閾値(
0.5)は名前付き定数にすること - 未使用の
register_strategy/strategiesは YAGNI に基づき削除すること(複数返金方式が実際に必要になった時点で第8章のストラテジパターンを導入する方針をコメントで明記すればよい) - Google スタイル docstring・インラインコメント・名前付き定数を含めること
悪いコード (Before)
class RefundHandler:
"""返金処理ハンドラ"""
def __init__(self, gateway, db):
self.gateway = gateway
self.db = db
# 問題4: 将来の複数返金方式のために用意したが、実際には呼ばれていない
self.strategies = {}
def register_strategy(self, key, strategy):
self.strategies[key] = strategy
def process(self, order_id, amount, reason_code):
try:
order = self.db.get_order(order_id)
if order is None:
return None # 問題2: 「注文なし」が None
# 問題1: 3 が何を意味するかコードだけでは分からない
if order["status"] != 3:
return -1 # 問題2: 「対象外」が -1
# 問題1: 0.5, 1 もマジックナンバー
if amount > order["total"] * 0.5:
if reason_code != 1:
return -1 # 問題2: これも同じ -1(バリデーションNG)
refund_id = self.gateway.refund(order_id, amount)
self.db.update_status(order_id, 4)
return refund_id
except Exception:
# 問題3: 決済ゲートウェイの例外を無言で握り潰す(ログなし)
return None
def handle_return_request(handler, order_id, amount, reason_code):
result = handler.process(order_id, amount, reason_code)
if result is None:
# 問題5: 「注文なし」なのか「決済障害」なのか区別できない
notify_ops_team(f"{order_id} の返金に失敗しました(原因不明)")
elif result == -1:
# 問題6: 「未配送」なのか「上限超過」なのか区別できない
pass
else:
mark_return_completed(order_id, result)
3, 4, 1, 0.5 が生の数値で埋め込まれ、意味を知るには別途仕様書が必要None、「未配送」「バリデーションNG」がどちらも -1 で区別不能except Exception: return None でログ・スタックトレースが残らず、決済障害が無言で消えるregister_strategy/strategies は未使用の投機的な抽象化refund_id(型不明)、失敗時は None または int(-1) と契約が曖昧handle_return_request が result is None/result == -1 でしか分岐できず、適切な対応(アラート先・再試行可否)を選べないNone を返すため、返金が実際に実行されたか失敗したかを呼び出し側が判別できず、安全な二重返金防止ができないヒント(段階的開示)
ヒント1 — 方向性
RefundHandler.process() の戻り値だけを見て、「注文が存在しない」場合と「決済ゲートウェイが落ちた」場合を呼び出し側はどうやって区別すればいいだろうか? どちらも None が返ってくる。同様に、「配送前だから対象外」なのか「金額が上限を超えていて理由コードが不適合だから対象外」なのかも、どちらも -1 で表現されていて区別できない。これは第10章でいう「null問題」(本来複数ある意味を1つの値に圧縮してしまう)そのもの。加えて register_strategy/strategies が実際に一度も呼ばれていないなら、それは「使われるかもしれない」ために先回りして作った抽象化=YAGNI違反の兆候。
ヒント2 — アプローチ
order["status"] != 3/reason_code != 1→OrderStatus(StrEnum)/RefundReasonCode(StrEnum)にして、生の整数はDBから取得した直後に変換するreturn None/return -1の3種類の意味 →OrderNotFoundError/RefundNotEligibleError/PaymentGatewayErrorという専用の例外階層に分離し、成功時のみRefundResult(値オブジェクト)を返すexcept Exception: return None→except Exception as exc: raise PaymentGatewayError(...) from excにして、原因の例外を握り潰さず保持するamount > order["total"] * 0.5の0.5→MAX_PARTIAL_REFUND_RATIOという名前付き定数にするself.strategies = {}/register_strategy→ 実際に使われていないなら丸ごと削除。複数方式が必要になったら第8章のStrategyパターンで対応する、という方針だけコメントに残す
ヒント3 — コードの骨格
class OrderStatus(StrEnum):
PENDING = "pending"
DELIVERED = "delivered"
REFUNDED = "refunded"
class RefundReasonCode(StrEnum):
QUALITY_DEFECT = "quality_defect"
SHIPPING_DAMAGE = "shipping_damage"
CUSTOMER_CHANGE_OF_MIND = "customer_change_of_mind"
class OrderNotFoundError(Exception): ...
class RefundNotEligibleError(Exception): ...
class PaymentGatewayError(Exception): ...
@dataclass(frozen=True, slots=True)
class RefundResult:
order_id: str
refund_id: str
amount: int
status: OrderStatus
class RefundProcessor:
def process(self, order_id: str, amount: int, reason_code: RefundReasonCode) -> RefundResult:
... # 失敗は raise、成功時のみ RefundResult を return
問題点分析(7点)
| # | 問題点 | 分類 | 改善方法 |
|---|---|---|---|
| 1 | status(3)/reason(1)/0.5 のマジックナンバー | マジックナンバー Ch10 | OrderStatus/RefundReasonCode(StrEnum) + MAX_PARTIAL_REFUND_RATIO |
| 2 | None/-1 が3つの異なる失敗を表す(多義性) | null問題 Ch10 | OrderNotFoundError/RefundNotEligibleError |
| 3 | except Exception: return None(握り潰し) | 例外の握り潰し Ch10 | raise PaymentGatewayError(...) from exc |
| 4 | 未使用の register_strategy/strategies | YAGNI Ch10 | 削除(必要時にCh8ストラテジパターン導入) |
| 5 | 戻り値の型が呼び出しごとに不定 | 契約不明確 Ch10 | -> RefundResult + 例外で失敗を表現 |
| 6 | 呼び出し側が原因を判別できない | null問題 Ch10 | 例外ごとの except 節で分岐 |
| 7 | 実行結果不明でリトライ・冪等性が担保できない | 運用リスク | 専用例外 + 構造化ログで追跡可能に |
リファクタ構造図(SVG)
模範解答
class RefundHandler:
"""返金処理ハンドラ"""
def __init__(self, gateway, db):
self.gateway = gateway
self.db = db
self.strategies = {}
def register_strategy(self, key, strategy):
self.strategies[key] = strategy
def process(self, order_id, amount, reason_code):
try:
order = self.db.get_order(order_id)
if order is None:
return None
if order["status"] != 3:
return -1
if amount > order["total"] * 0.5:
if reason_code != 1:
return -1
refund_id = self.gateway.refund(order_id, amount)
self.db.update_status(order_id, 4)
return refund_id
except Exception:
return None
def handle_return_request(handler, order_id, amount, reason_code):
result = handler.process(order_id, amount, reason_code)
if result is None:
notify_ops_team(f"{order_id} の返金に失敗しました(原因不明)")
elif result == -1:
pass
else:
mark_return_completed(order_id, result)
"""refund_processing.py — 返品承認後の返金処理
良いコード・悪いコードで学ぶ設計入門(改訂新版)
Ch10: 設計の悪魔 — マジックナンバー、null問題、例外の握り潰し、YAGNI
"""
from __future__ import annotations
from dataclasses import dataclass
from enum import StrEnum
class OrderStatus(StrEnum):
"""注文のライフサイクル状態。"""
PENDING = "pending"
DELIVERED = "delivered"
REFUNDED = "refunded"
class RefundReasonCode(StrEnum):
"""返金理由コード。"""
QUALITY_DEFECT = "quality_defect"
SHIPPING_DAMAGE = "shipping_damage"
CUSTOMER_CHANGE_OF_MIND = "customer_change_of_mind"
# レガシーDBが返す整数ステータスコードを型安全な OrderStatus に変換する対応表。
# 生の整数はこの境界(get_order の直後)でのみ扱い、その先には漏らさない。
LEGACY_STATUS_CODE_TO_ORDER_STATUS: dict[int, OrderStatus] = {
0: OrderStatus.PENDING,
3: OrderStatus.DELIVERED,
4: OrderStatus.REFUNDED,
}
# 返金額が注文金額に対してこの比率を超える場合、「上限超の返金」とみなす。
MAX_PARTIAL_REFUND_RATIO = 0.5
# 上限超の返金でも許可される理由コード。
ELIGIBLE_REASONS_FOR_LARGE_REFUND = frozenset(
{RefundReasonCode.QUALITY_DEFECT, RefundReasonCode.SHIPPING_DAMAGE}
)
class OrderNotFoundError(Exception):
"""指定された注文IDが存在しない場合に送出する。"""
class RefundNotEligibleError(Exception):
"""返金条件を満たさない場合に送出する(未配送、理由コード不適合など)。"""
class PaymentGatewayError(Exception):
"""決済ゲートウェイ呼び出しが失敗した場合に送出する。
原因の例外は `raise ... from exc` により `__cause__` に保持される。
"""
@dataclass(frozen=True, slots=True)
class RefundResult:
"""返金処理が成功したことを表す値オブジェクト。
Attributes:
order_id: 対象注文ID。
refund_id: 決済ゲートウェイが発行した返金ID。
amount: 返金額(円)。
status: 更新後の注文ステータス。
"""
order_id: str
refund_id: str
amount: int
status: OrderStatus
class RefundProcessor:
"""注文の返金処理を担当する。
Note:
返金方式(全額/一部/店舗クレジット等)は現時点で1種類のみ運用されている。
複数方式への対応が具体的に必要になった時点で第8章のストラテジパターンを
導入すればよく、必要性が確認できていない現状では投機的な抽象化を避ける
(YAGNI, 第10章)。旧実装にあった register_strategy/strategies は
一度も呼ばれていなかったため削除した。
"""
def __init__(self, gateway, db) -> None:
self._gateway = gateway
self._db = db
def process(
self, order_id: str, amount: int, reason_code: RefundReasonCode
) -> RefundResult:
"""返金を実行する。
Args:
order_id: 対象注文ID。
amount: 返金希望額(円)。
reason_code: 返金理由コード。
Returns:
成功時の RefundResult。
Raises:
OrderNotFoundError: 注文が存在しない場合。
RefundNotEligibleError: 未配送、または上限比率超で理由コードが
不適合な場合。
PaymentGatewayError: 決済ゲートウェイ呼び出しが失敗した場合。
"""
raw_order = self._db.get_order(order_id)
if raw_order is None:
raise OrderNotFoundError(f"order_id={order_id} が見つかりません")
status = LEGACY_STATUS_CODE_TO_ORDER_STATUS[raw_order["status"]]
if status is not OrderStatus.DELIVERED:
raise RefundNotEligibleError(
f"order_id={order_id} はステータス={status} のため返金対象外です"
)
if amount > raw_order["total"] * MAX_PARTIAL_REFUND_RATIO:
if reason_code not in ELIGIBLE_REASONS_FOR_LARGE_REFUND:
raise RefundNotEligibleError(
f"理由コード={reason_code} は半額超の返金には適用できません"
)
try:
refund_id = self._gateway.refund(order_id, amount)
except Exception as exc:
# 原因の例外を cause として保持したまま専用例外に変換する。
# ログ・監視基盤(DataDog等)が根本原因まで追跡できるようにする。
raise PaymentGatewayError(
f"order_id={order_id} の決済ゲートウェイ呼び出しに失敗しました"
) from exc
self._db.update_status(order_id, OrderStatus.REFUNDED)
return RefundResult(
order_id=order_id,
refund_id=refund_id,
amount=amount,
status=OrderStatus.REFUNDED,
)
def handle_return_request(
processor: RefundProcessor,
order_id: str,
amount: int,
reason_code: RefundReasonCode,
) -> None:
"""返品申請を受けて返金を実行し、結果に応じて後続処理を行う。"""
try:
result = processor.process(order_id, amount, reason_code)
except OrderNotFoundError:
notify_ops_team(f"{order_id}: 注文が見つかりません")
except RefundNotEligibleError as exc:
notify_ops_team(f"{order_id}: 返金対象外 ({exc})")
except PaymentGatewayError as exc:
# exc.__cause__ に元の例外(ネットワークエラー等)が保持されている
notify_ops_team(f"{order_id}: 決済ゲートウェイ障害 ({exc})")
else:
mark_return_completed(result.order_id, result.refund_id)
processor = RefundProcessor(gateway=payment_gateway, db=order_repository)
# 正常系: 全額返金(気が変わったため)
result = processor.process("ORD-1001", 3000, RefundReasonCode.CUSTOMER_CHANGE_OF_MIND)
print(result)
# RefundResult(order_id='ORD-1001', refund_id='re_abc123', amount=3000, status=<OrderStatus.REFUNDED: 'refunded'>)
# 異常系: 未配送の注文
try:
processor.process("ORD-1002", 1000, RefundReasonCode.CUSTOMER_CHANGE_OF_MIND)
except RefundNotEligibleError as e:
print(e)
# order_id=ORD-1002 はステータス=OrderStatus.PENDING のため返金対象外です
# 異常系: 半額超だが理由コードが「気が変わった」→ 対象外
try:
processor.process("ORD-1003", 8000, RefundReasonCode.CUSTOMER_CHANGE_OF_MIND)
except RefundNotEligibleError as e:
print(e)
# 理由コード=RefundReasonCode.CUSTOMER_CHANGE_OF_MIND は半額超の返金には適用できません
# 異常系: 決済ゲートウェイ障害(原因の例外を保持)
try:
processor.process("ORD-1004", 5000, RefundReasonCode.SHIPPING_DAMAGE)
except PaymentGatewayError as e:
print(e) # order_id=ORD-1004 の決済ゲートウェイ呼び出しに失敗しました
print(e.__cause__) # 元のネットワークエラー等がそのまま確認できる
| ポイント | 適用した設計原則 | 書籍対応章 |
|---|---|---|
status != 3 / reason_code != 1 → OrderStatus/RefundReasonCode(StrEnum) | マジックナンバーの排除 | Ch10 |
0.5 → MAX_PARTIAL_REFUND_RATIO | マジックナンバーの名前付き定数化 | Ch10 |
None/-1(3つの意味)→ 専用例外3種 | null問題の解消 | Ch10 |
except Exception: return None → raise ... from exc | 例外の握り潰し防止 | Ch10 |
register_strategy/strategies(未使用)の削除 | YAGNI | Ch10 |
RefundResult を dataclass(frozen=True, slots=True) 化 | 値オブジェクト・不変の活用 | Ch3/Ch4 |
# tests/test_refund_processing.py
import pytest
from refund_processing import (
OrderNotFoundError,
PaymentGatewayError,
RefundNotEligibleError,
RefundProcessor,
RefundReasonCode,
)
class FakeGateway:
def __init__(self, should_fail=False):
self.should_fail = should_fail
def refund(self, order_id, amount):
if self.should_fail:
raise ConnectionError("timeout")
return "re_abc123"
class FakeRepository:
def __init__(self, order=None):
self._order = order
self.updated_status = None
def get_order(self, order_id):
return self._order
def update_status(self, order_id, status):
self.updated_status = status
class TestRefundProcessor:
def test_process_returns_result_on_success(self):
db = FakeRepository(order={"status": 3, "total": 5000})
processor = RefundProcessor(gateway=FakeGateway(), db=db)
result = processor.process("ORD-1", 3000, RefundReasonCode.CUSTOMER_CHANGE_OF_MIND)
assert result.refund_id == "re_abc123"
assert db.updated_status is not None
def test_process_raises_order_not_found(self):
db = FakeRepository(order=None)
processor = RefundProcessor(gateway=FakeGateway(), db=db)
with pytest.raises(OrderNotFoundError):
processor.process("ORD-404", 1000, RefundReasonCode.SHIPPING_DAMAGE)
def test_process_raises_not_eligible_when_not_delivered(self):
db = FakeRepository(order={"status": 0, "total": 5000})
processor = RefundProcessor(gateway=FakeGateway(), db=db)
with pytest.raises(RefundNotEligibleError):
processor.process("ORD-2", 1000, RefundReasonCode.SHIPPING_DAMAGE)
def test_process_raises_not_eligible_when_reason_not_allowed_for_large_refund(self):
db = FakeRepository(order={"status": 3, "total": 5000})
processor = RefundProcessor(gateway=FakeGateway(), db=db)
with pytest.raises(RefundNotEligibleError):
processor.process("ORD-3", 4000, RefundReasonCode.CUSTOMER_CHANGE_OF_MIND)
def test_process_wraps_gateway_exception_with_cause_preserved(self):
db = FakeRepository(order={"status": 3, "total": 5000})
processor = RefundProcessor(gateway=FakeGateway(should_fail=True), db=db)
with pytest.raises(PaymentGatewayError) as exc_info:
processor.process("ORD-4", 1000, RefundReasonCode.SHIPPING_DAMAGE)
assert isinstance(exc_info.value.__cause__, ConnectionError)
ポイント解説
status != 3 や reason_code != 1 は、コードを読むだけでは「3が何を意味するか」が分からず、DBのステータス定義書を別途参照しないと理解できない。OrderStatus.DELIVERED にすることで、コード自体がドキュメントになる。閾値 0.5 も MAX_PARTIAL_REFUND_RATIO という名前を与えることで、変更時の影響範囲が名前検索だけで把握できる。None、「未配送」「バリデーションNG」がどちらも -1 という、本来別々であるべき失敗理由が2種類の値に押し込まれていた。呼び出し側は原因ごとに異なる対応(アラート先・再試行可否)を取りたいのに、戻り値からはそれができない。専用の例外クラスに分けることで、except 節ごとに適切な処理を書け、かつ型チェッカーが「起こりうる失敗」を網羅性チェックの対象にできる。except Exception: return None は、決済ゲートウェイのネットワークエラーもタイムアウトも認証エラーも同じ「None」に変換してしまい、ログにもスタックトレースが残らない。障害調査時に「何が起きたか」を追跡する手段が失われる。raise PaymentGatewayError(...) from exc にすることで、専用例外としての扱いやすさと、元の例外(__cause__)による根本原因追跡の両方を両立できる。register_strategy/strategies は「将来複数の返金方式に対応するため」という理由で先回りして作られていたが、実際に呼ばれる方式は1つしかなかった。使われない抽象化は読み手に「これは本当に使われているのか、探さないと分からない」という認知負荷を強いるだけで、価値を生まない。必要になった時点で第8章のストラテジパターンを導入すればよく、今は削除するのが正しい判断。gateway.refund() の戻り値(型不明)、失敗時は None または -1 と、1つのメソッドが呼び出しごとに異なる型を返していた。After では成功時のみ RefundResult を返し、失敗は例外で表現することで、process() の型シグネチャ -> RefundResult がそのまま「呼べたら必ずこの形の値が返る」という契約になる。実務への応用
MOps チームの返品/返金処理バッチでは、決済ゲートウェイの一時的な障害(ネットワークタイムアウト、レート制限)が except Exception: return None で握り潰されると、返金が実際には実行されていないのに「何も起きなかったこと」として処理が終わってしまい、在庫・会計システムとの突合せが後日ズレて発覚するという事故につながりやすい。専用例外 + raise ... from exc で原因を保持しておけば、DataDog のエラートラッキングやログの exc_info から根本原因(ゲートウェイ側の障害かバリデーションミスか)をすぐに切り分けられる。
今日のまとめ
次のステップ
- 発展問題: 実際に返金方式が「全額」「一部」「店舗クレジット」の3種類に増える要件が確定した場合を想定し、あえて導入しなかった第8章のストラテジパターンへ移行するリファクタリング手順(既存
RefundProcessor.process()からの段階的な抽出方法とテストケース設計)を設計してください。 - 参考: 「良いコード・悪いコードで学ぶ設計入門(改訂新版)」Ch10(設計の悪魔)/ Ch8(ストラテジパターン)/ YAGNI原則 / Python の例外連鎖(
raise ... from)公式ドキュメント