社内コードレビュー勉強会のまとめ記事
先日、社内でコードレビューに関する勉強会を実施する機会がありました。内容としては基礎的なものですが、意外と見落としている点も多く、良い学びになったと思います。
今回の記事ではコードレビュー勉強会の内容をまとめ、備忘録的なものとして作成しようと考えています。コードレビューが苦手な方や、勉強会に参加できなかった社内の方の助けに少しでもなれば幸いです。
Table of Contents
イントロダクション
みなさん、コードレビューは得意ですか？ おそらく「得意」と自信を持って答えられる方は少ないのではないでしょうか。他の人が実装した機能や書いたコードを理解し、確認することは、認知負荷が非常に高く、負担に感じることが多い作業です。
もう一つ質問をさせてください。あなたのレビューは動作テストだけで終わっていませんか？ もちろん、コードが意図通り動作することは大切です。しかし、それだけで十分とは言えません。読みやすく、意図が明確に伝わるコードであるかどうかを意識することで、より質の高いレビューが可能になります。
Martin Fowler 著『リファクタリング 第2版』では、リファクタリングを促すような改善の余地があるコードを「不吉な臭い」として表現しています。この「コードの臭い」を嗅ぎ分ける「嗅覚」は、経験により培われる部分もありますが、まずは具体的なパターンを知ることが重要です。
今回はそのとっかかりとして、どのようなコードが問題を抱え、どのように改善できるのかを具体的な例を通して学んでみましょう。
改善の余地が含まれるサンプルコード
今回の題材は、フランスのECサイトにおける注文料金の算出処理です。このコードはすでに正しく動作しますが、改善すべきポイントが複数含まれています。なお、ここではコード内で参照されているクラスの定義などは省略しています。また、特定のフレームワークや言語特有の機能には言及せず、プログラミングの一般的なふるまいに着目しています。
実際のコードレビューでは、何往復かして改善点を洗い出すこともよくありますが、今回はあえて全部先回りして指摘するくらいの気持ちで見ていきましょう。
class Order
{
/** @var OrderItem[] */
private readonly array $items;
private readonly User $user;
private readonly Address $address;
/**
* @param OrderItem[] $items
*/
public function __construct(array $items, User $user, Address $address)
{
$this->items = $items;
$this->user = $user;
$this->address = $address;
}
public function total(): float
{
// 小計を計算する
$subtotal = 0;
foreach ($this->items as $item) {
// VAT (消費税) を加えた価格を計算
// 商品カテゴリによって軽減税率が適用される
$totalPrice = $item->product->price * $item->quantity;
$vat = match ($item->product->category) {
'food' => 0.055,
'book' => 0.055,
'medicine' => 0.055,
'newspaper' => 0.021,
'news_magazine' => 0.021,
default => 0.2,
};
$subtotal += $totalPrice * (1 + $vat);
}
// 送料を計算する
$shippingFee = 0;
if ($this->address->country === 'France') {
if ($this->user->isVip) {
$shippingFee = 0;
} else {
$shippingFee = $subtotal >= 35 ? 0 : 3.99;
}
} else if ($this->address->isInEurope) {
$shippingFee = 6.99;
} else {
$shippingFee = 19.99;
}
// フランス国内の配送では、新品の書籍に一定の配送料を付加しなければならない
$newBookTotal = null;
foreach ($this->items as $item) {
if ($item->product->category === 'book' && ! $item->product->isUsed) {
// VAT (消費税) を加えた価格を計算
// 商品カテゴリによって軽減税率が適用される
$totalPrice = $item->product->price * $item->quantity;
$vat = match ($item->product->category) {
'food' => 0.055,
'book' => 0.055,
'medicine' => 0.055,
'newspaper' => 0.021,
'news_magazine' => 0.021,
default => 0.2,
};
$newBookTotal = $newBookTotal ?? 0;
$newBookTotal += $totalPrice * (1 + $vat);
}
}
if ($newBookTotal !== null && $this->address->country === 'France') {
$newBookShippingFee = $newBookTotal >= 35 ? 3 : 0.01;
}
return $subtotal + $shippingFee + $newBookShippingFee;
}
}
具体的な改善案
今回提示したサンプルコードの改善点を具体的に見ていきましょう。
関数の抽出
- 提示されたコードの
totalメソッドは非常に長く、複数の責務を持っています。 - 処理が長くなるとコメントで章立てして整理したくなりますが、それはまさに関数抽出の目安です。単一の責務を持つ関数に分割しましょう。
マジックナンバーなどの定数化
- 直接コードに書かれた数値（マジックナンバー）は、定数化することでコードの意図が明確になります。
- 列挙型を使用できる言語の場合は、積極的に列挙型を活用しましょう。
コレクション操作関数の活用
- 多くのプログラミング言語では、配列やリストを便利に操作できる関数やメソッドが用意されています（Goのように意図的にシンプルさを追求して提供していない例外はあります）。
forループよりもmapやreduceなどのコレクション操作関数を使うことで、コードの意図が明確になり、読みやすくなります。
ガード節 (早期リターン) の活用
- 条件分岐のブロックが長くなったり、ネストが深くなっている場合、ガード節を活用して例外的な処理を早期リターンすることを検討しましょう。
- これにより、コードの可読性が向上します。
実際に改善を行ったコードは以下の通りです。
class Order
{
private const VAT_RATES = [
'food' => 0.055,
'book' => 0.055,
'medicine' => 0.055,
'newspaper' => 0.021,
'news_magazine' => 0.021,
'default' => 0.2,
];
private const DOMESTIC_SHIPPING_THRESHOLD = 35.0;
private const DOMESTIC_SHIPPING_FEE_BELOW_THRESHOLD = 3.99;
private const EUROPE_SHIPPING_FEE = 6.99;
private const INTERNATIONAL_SHIPPING_FEE = 19.99;
private const NEW_BOOK_SHIPPING_THRESHOLD = 35.0;
private const NEW_BOOK_SHIPPING_FEE_BELOW_THRESHOLD = 0.01;
private const NEW_BOOK_SHIPPING_FEE_ABOVE_THRESHOLD = 3.0;
private const COUNTRY_FRANCE = 'France';
/** @var OrderItem[] */
private array $items;
private User $user;
private Address $address;
/**
* @param OrderItem[] $items
*/
public function __construct(array $items, User $user, Address $address)
{
$this->items = $items;
$this->user = $user;
$this->address = $address;
}
public function total(): float
{
$subtotal = $this->calculateSubtotal();
$shippingFee = $this->calculateShippingFee($subtotal);
$newBookShippingFee = $this->calculateNewBookShippingFee();
return $subtotal + $shippingFee + $newBookShippingFee;
}
private function calculateSubtotal(): float
{
return array_reduce($this->items, function (float $carry, OrderItem $item): float {
$totalPrice = $item->product->price * $item->quantity;
$vatRate = self::VAT_RATES[$item->product->category] ?? self::VAT_RATES['default'];
return $carry + $totalPrice * (1 + $vatRate);
}, 0.0);
}
private function calculateShippingFee(float $subtotal): float
{
if ($this->address->country === self::COUNTRY_FRANCE) {
if ($this->user->isVip) {
return 0.0;
}
return $subtotal >= self::DOMESTIC_SHIPPING_THRESHOLD
? 0.0
: self::DOMESTIC_SHIPPING_FEE_BELOW_THRESHOLD;
}
if ($this->address->isInEurope) {
return self::EUROPE_SHIPPING_FEE;
}
return self::INTERNATIONAL_SHIPPING_FEE;
}
private function calculateNewBookShippingFee(): float
{
if ($this->address->country !== self::COUNTRY_FRANCE) {
return 0.0;
}
$newBookItems = array_filter($this->items, fn(OrderItem $item) =>
$item->product->category === 'book' && !$item->product->isUsed
);
if (empty($newBookItems)) {
return 0.0;
}
$newBookTotal = array_reduce($newBookItems, function (float $carry, OrderItem $item): float {
$totalPrice = $item->product->price * $item->quantity;
$vatRate = self::VAT_RATES[$item->product->category] ?? self::VAT_RATES['default'];
return $carry + $totalPrice * (1 + $vatRate);
}, 0.0);
return $newBookTotal >= self::NEW_BOOK_SHIPPING_THRESHOLD
? self::NEW_BOOK_SHIPPING_FEE_ABOVE_THRESHOLD
: self::NEW_BOOK_SHIPPING_FEE_BELOW_THRESHOLD;
}
}
改善点の詳細
- 関数の抽出:
totalメソッド内の処理を、calculateSubtotal、calculateShippingFee、calculateNewBookShippingFeeの3つのメソッドに分割しました。これにより、各メソッドが単一の責務を持ち、コードの可読性と保守性が向上しました。
- マジックナンバーの定数化:
- VAT率や送料などの数値をクラス定数として定義しました。これにより、コード内の数値が意味を持つ名前で参照され、理解しやすくなりました。
- コレクション操作関数の活用:
array_reduceを使用して、小計や新品書籍の合計を計算しています。これにより、ループ処理が簡潔になり、意図が明確になりました。また、array_filterとarray_reduceを分けることでコードの意図を明確にしています。
- ガード節の活用:
calculateNewBookShippingFeeメソッド内で、フランス国外の場合に早期リターンを行うことで、ネストを減らし、コードの可読性を向上させています。
- ハードコーディングの改善
- 国名のような繰り返し使用される文字列を定数化することで、タイポのリスクを軽減し、コードの安全性と保守性を高めました。
これらの改善により、コードの可読性、保守性、拡張性が大幅に向上しました。関数の抽出や定数の活用、コレクション操作関数の適切な使用、ガード節の導入は、複雑なビジネスロジックを持つコードを整理し、理解しやすくする上で非常に有効な手法です。
もちろん、どこまでリファクタリングを行うかはプロジェクトの規模や状況、開発体制によって異なります。そのため、現在のコードでも十分に問題なく運用できるケースも多く存在します。ただし、今回のように一見改善されたように見えるコードであっても、テストのしやすさ（テスタビリティ）という観点からは、さらに改善の余地があると考えられます。
特に、ビジネスロジックが Order クラスに集約されているため、単体テストを書く際にモックやスタブが複雑になりやすく、今後の保守や仕様変更に対して柔軟に対応しづらい可能性があります。
最後に
プログラマの仕事の多くは、「コードを読む」ことに労力を割いています（参考記事）。また、一度書かれたプログラムは、皆さんが思うよりずっと長く使われ続けます。プロジェクトが継続するとコードには継ぎ接ぎの修正が加えられ、完全に書き換えられるケースはほとんどありません。プログラミング言語やシステムの移管があったとしても、元のコードは必ず参考にされます。
さらに、プロジェクト終了後も他のプロジェクトにコードが流用されることはよくあります。そのため、自分でコードを書くときも、他人のコードをレビューするときも、「読みやすく、意図が明確なコード」を心がけることがとても重要です。ぜひ、この意識を日々の開発に取り入れていきましょう。
コメントを残す