【入門】結局getter/setterは悪なのか
先に結論
- setterは悪。
- getterは必要なケースがあり、一概に悪といえない。
- getterによりビジネスロジックをオブジェクト外に流出させてしまうのは悪。
想定読者
- オブジェクト指向に入門した方々
- getter/setterがNGというのは概念的に理解したけど、具体的な改善方法を知りたい方々
- というか1年前の僕
ネットでgetter/setterについて検索すると、「必要ない」だとか「カプセル化を破る」だとか「Tell, Don't Ask」だとかが出てきます。
getter setter 使わない - Google Search
普通の人なら「getter/setterはアンチパターンなのだ」と認識することでしょう。
しかし、現場のコードを見ると普通に使われてたりします。
特にgetterは検索結果など知らんという顔で歩いており、オブジェクト指向に入門した方は非常に困惑すると思います。
また、オブジェクト指向の説明はどうしても概念的になりやすい、という側面を持っています。
getter/setterに関しても、実装による説明は控えめで、文章での説明が多くなってしまいます。
getter/setterを使うな、と言われても入門者にとっては「じゃあどうすれば良いの?」という感じだと思います。
少なくとも1年前の僕は困惑しました。 そんな方々に届けばと思って書きました。
本記事ではPHPによる実装を通じて、悪いgetter/setterを表現し、それが後々どう問題になってしまうのか、どう改善すれば良いのかを説明したいと思います。
なぜgetter/setterの善悪議論は終着しないのか
少し論点を整理したいと思います。
- getterとsetterの善悪を一律で判断できない
- getterは悪とされるパターンとそうでないパターンが存在する
まずgetterとsetterはそれぞれオブジェクトへの読み込みと書き込みであり、性質が全く異なるものです。
単純なgetterであれば何度読み込んでも副作用はないはずである一方、setterは書き込みという副作用があります。
この2つを同列で扱って議論すると、議論が終着しなくなってしまいます。
setterはコンストラクタ等でせっかく満たしていたオブジェクト生成条件を壊して、mutableなオブジェクトを作れてしまいます。
その結果、処理が理解しにくくなったり、思わぬバグを生み出す原因になることがあります。
本記事の意見としてはsetterは「悪」としています。
一方でgetterには悪となるケースとそうでないケースがあります。
getterは使い方を間違えると「悪」となるケースがあり、それはビジネスロジックがオブジェクトの外に流出しているケースです。
この場合、ビジネスロジックが書かれている場所が分散してしまい、ビジネスロジックの可読性・再利用性などを低下させてしまいます。
ではgetterは必ず悪なのかというと、インターフェース層などでプリミティブな値を出力するケースで必要になることがあります。
つまり、getterを一律で「不要」「悪」と断じることはできません[1]。
これらについて、具体的にPHPの実装を元に説明できればと思ってます。
実装
バージョン
php -v
PHP 8.0.0rc1 (cli) (built: Oct 13 2020 08:42:44) ( NTS )
Copyright (c) The PHP Group
Zend Engine v4.0.0-dev, Copyright (c) Zend Technologies
悪いgetterとsetterを使った実装
仕様1,2
エンジニアのAさんは、こんな仕様の関数を作ることになりました。
- アイテムの単価と数量を引数とし、合計額を計算する
- 計算に使った単価と数量はオーダー日時とともにアイテムオーダーとして保存する
- 単価か数量が0以下の場合はエラーとなる
- エラーの場合は0を返す
実装は以下となりました。
ちなみに、この段階ではまだそこまで悪い実装にはなっていません。
<?php
function calc_total_price(int $unitPrice, int $quantity): int
{
$itemOrder = new ItemOrder($unitPrice, $quantity, new DateTimeImmutable());
if (!$itemOrder->validates()) {
return 0;
}
$itemOrderRepository = new ItemOrderRepository();
if (!$itemOrderRepository->persist($itemOrder)) {
return 0;
}
return $itemOrder->getUnitPrice() * $itemOrder->getQuantity();
}
// 仕様1: 単価*個数が返ってくること
assert(calc_total_price(200, 4) === 800);
// 仕様2: 単価がマイナスではエラーとなること
assert(calc_total_price(-500, 4) === 0);
echo 'all green' . PHP_EOL;
class ItemOrder
{
private int $unitPrice;
private int $quantity;
private DateTimeImmutable $orderedAt;
public function __construct(int $unitPrice, int $quantity, DateTimeImmutable $orderedAt)
{
$this->unitPrice = $unitPrice;
$this->quantity = $quantity;
$this->orderedAt = $orderedAt;
}
public function getUnitPrice(): int
{
return $this->unitPrice;
}
public function getQuantity(): int
{
return $this->quantity;
}
public function getOrderedAt(): DateTimeImmutable
{
return $this->orderedAt;
}
public function validates(): bool
{
return 0 < $this->unitPrice && 0 < $this->quantity;
}
}
class ItemOrderRepository
{
public function persist(ItemOrder $itemOrder): bool
{
$params = [
'unit_price' => $itemOrder->getUnitPrice(),
'quantity' => $itemOrder->getQuantity(),
'ordered_at' => $itemOrder->getOrderedAt()->getTimestamp(),
];
// アイテムオーダーをDBとかに保存する処理が書かれてるとします
return true;
}
}
php calc.php
all green
いくつか実装について説明します。
- テストは
assert
関数によって表現してます[2]。 -
calc_total_price
関数が本体で、この中でDBへの保存と合計額を返すという処理を実行します。 -
ItemOrder
クラスは単価と数量とオーダー日時を持ったオブジェクトを作ります[3]。 -
ItemOrderRepository
クラスについてはItemOrder
オブジェクトのDBへの永続化を担っている、という程度の認識で大丈夫です。あまり理解してなくても本記事の趣旨を理解するのには影響しません[4]。 -
validates
関数も実装しているので、単価や数量にマイナス値とかが入ることはなくAさんも安心しました(伏線)
仕様は満たせてますし、一旦はこのまま進めます。
仕様3
追加の仕様が来ました。
- 個数が5個以上なら単価は20円引きとなる
たくさん買ってくれる場合には割引することで個数UPを促進する、というよくある追加仕様かと思います。
以下のように実装しました。
<?php
function calc_total_price(int $unitPrice, int $quantity): int
{
$itemOrder = new ItemOrder($unitPrice, $quantity, new DateTimeImmutable());
if (!$itemOrder->validates()) {
return 0;
}
+ if (5 <= $itemOrder->getQuantity()) {
+ $discountedPrice = $itemOrder->getUnitPrice() - 20;
+ $itemOrder->setUnitPrice($discountedPrice);
+ }
+
$itemOrderRepository = new ItemOrderRepository();
if (!$itemOrderRepository->persist($itemOrder)) {
return 0;
}
return $itemOrder->getUnitPrice() * $itemOrder->getQuantity();
}
// 仕様1: 単価*個数が返ってくること
assert(calc_total_price(200, 4) === 800);
// 仕様2: 単価がマイナスではエラーとなること
assert(calc_total_price(-500, 4) === 0);
+// 仕様3: 個数が5個以上なら単価は20円引きとなること
+assert(calc_total_price(200, 5) === 900);
+
echo 'all green' . PHP_EOL;
class ItemOrder
{
private int $unitPrice;
private int $quantity;
private DateTimeImmutable $orderedAt;
public function __construct(int $unitPrice, int $quantity, DateTimeImmutable $orderedAt)
{
$this->unitPrice = $unitPrice;
$this->quantity = $quantity;
$this->orderedAt = $orderedAt;
}
public function getUnitPrice(): int
{
return $this->unitPrice;
}
public function getQuantity(): int
{
return $this->quantity;
}
public function getOrderedAt(): DateTimeImmutable
{
return $this->orderedAt;
}
public function validates(): bool
{
return 0 < $this->unitPrice && 0 < $this->quantity;
}
+
+ public function setUnitPrice(int $unitPrice)
+ {
+ $this->unitPrice = $unitPrice;
+ }
}
(中略)
php calc.php
all green
実装について説明すると以下のようになります。
- 個数が5個以上のときに単価を下げる、という処理にsetterを使いました。
- 個数が5個以上かどうかは
calc_total_price
関数にベタ書きしました。
Aさんはテストも通ったので安心しました(伏線)。
しかし、実はこの時点でバグがあります。皆さんは分かりますでしょうか?
仕様4,5
この改修をリリースしてすぐ、問題が発覚しました。
(略)
// 仕様1: 単価*個数が返ってくること
assert(calc_total_price(200, 4) === 800);
// 仕様2: 単価がマイナスではエラーとなること
assert(calc_total_price(-500, 4) === 0);
// 仕様3: 個数が5個以上なら単価は20円引きとなること
assert(calc_total_price(200, 5) === 900);
+// バグ: 合計額がマイナスになる!?
+echo calc_total_price(15, 10) . PHP_EOL;
+
echo 'all green' . PHP_EOL;
(中略)
php calc.php
-50
all green
なんと引数となる単価が20円以下かつ個数が5個以上のケースで、DBに保存される単価および合計金額がマイナスになってしまいます。
ちゃんとマイナスにならないようにvalidates
関数を入れたのに何故・・・と思ったところでAさんは処理順が間違えていたことに気づきました。
この設計の場合、バリデーションはDBに保存するギリギリで行うべきですが、バリデーション後にsetterを呼んでしまっています。
これではバリエーションの意味がなくなってしまっています。
よってAさんは処理順を修正することにしました。
加えて、そもそものビジネス上の仕様として、単価が20円以下かつ個数が5個以上のケースが考慮されていないことに気付きます。
Aさんは仕様のミスだと怒ることによって、体裁を保つことができました[5]。
以下の仕様を追加することになりました。
- 単価が100円以下のときには単価の値引きしない
また、合計額についても値引きの追加仕様が入ることになりました。
- 合計額が3000円以上のときには合計額は300円引きとなる
Aさんはちょっとバグが怖いなーと言いながら実装し、最終的に以下の実装となりました。
<?php
function calc_total_price(int $unitPrice, int $quantity): int
{
$itemOrder = new ItemOrder($unitPrice, $quantity, new DateTimeImmutable());
- if (!$itemOrder->validates()) {
- return 0;
- }
-
- if (5 <= $itemOrder->getQuantity()) {
+ if (5 <= $itemOrder->getQuantity() && 100 < $itemOrder->getUnitPrice()) {
$discountedPrice = $itemOrder->getUnitPrice() - 20;
$itemOrder->setUnitPrice($discountedPrice);
}
+ if (!$itemOrder->validates()) {
+ return 0;
+ }
+
$itemOrderRepository = new ItemOrderRepository();
if (!$itemOrderRepository->persist($itemOrder)) {
return 0;
}
- return $itemOrder->getUnitPrice() * $itemOrder->getQuantity();
+ $totalPrice = $itemOrder->getUnitPrice() * $itemOrder->getQuantity();
+ if (3000 <= $totalPrice) {
+ $totalPrice = $totalPrice - 300;
+ }
+
+ return $totalPrice;
}
// 仕様1: 単価*個数が返ってくること
assert(calc_total_price(200, 4) === 800);
// 仕様2: 単価がマイナスではエラーとなること
assert(calc_total_price(-500, 4) === 0);
// 仕様3: 個数が5個以上なら単価は20円引きとなること
assert(calc_total_price(200, 5) === 900);
// バグ: 合計額がマイナスになる!?
echo calc_total_price(15, 10) . PHP_EOL;
+// 仕様4: 単価が100円以下のときには単価の値引きをしないこと
+assert(calc_total_price(10, 5) === 50);
+
+// 仕様5: 合計額が3000円以上のときには合計額は300円引きとなること
+assert(calc_total_price(1000, 4) === 3700);
+
echo 'all green' . PHP_EOL;
(中略)
php calc.php
150
all green
この実装にはとりあえずバグはありません。
ただ、最初は簡単な処理であったはずのcalc_total_price
がいつのまにか結構太ってきています。
Aさんはこう言ってます。でも仕様を満たしてるから別に良いですよね?(伏線)
追加の関数
新しく関数を追加することになりました。
- アイテムの合計額が10000円以上にならないかを確認する[6]
- 確認するだけで、DBには保存しない
「基本的には前の処理を使えば良いはずだから簡単ですよね?」と言われてしまいました。
Aさんも最初は単純にcalc_total_price
関数を使うだけで良いと思っていましたが、calc_total_price
関数内のDBへの保存が邪魔していることに気付きました。
仕方なく以下の実装にしました。
<?php
require_once 'calc.php';
function is_over(int $unitPrice, int $quantity): bool
{
$itemOrder = new ItemOrder($unitPrice, $quantity, new DateTimeImmutable());
// また同じような処理を書いてる・・・
if (5 <= $itemOrder->getQuantity() && 100 < $itemOrder->getUnitPrice()) {
$discountedPrice = $itemOrder->getUnitPrice() - 20;
$itemOrder->setUnitPrice($discountedPrice);
}
if (!$itemOrder->validates()) {
return false;
}
$totalPrice = $itemOrder->getUnitPrice() * $itemOrder->getQuantity();
if (3000 <= $totalPrice) {
$totalPrice = $totalPrice - 300;
}
return 10000 <= $totalPrice;
}
// 仕様1: 合計額が10000円未満 (1000 - 20) * 10 - 300 = 9500 < 10000
assert(is_over(1000, 10) === false);
// 仕様2: 合計額が10000円 (1050 - 20) * 10 - 300 >= 10000
assert(is_over(1050, 10) === true);
echo 'all green' . PHP_EOL;
php is_over.php
all green
なんとcalc_total_price
関数をほぼコピペして実装してしまいました。
もちろんAさんもDRY原則は知っていましたが、もう色々考えるのが面倒になってしまったようです。
何が問題だったか
この記事はgetterとsetterについての記事です[7]。
Aさんの設計は以下の2つの問題点があります。
setterによってオブジェクトを破壊してしまっている
$discountedPrice = $itemOrder->getUnitPrice() - 20;
$itemOrder->setUnitPrice($discountedPrice);
Aさんは仕様3を満たすためにsetterを導入しました。
しかし、結果的にはオブジェクトの正しい状態を壊してしまい、バグを発生させてしまいました。
バリデーションの処理順を変えることで一旦は解決しているように見えますが、バリデーションとsetterの順番という暗黙的な約束を作ってしまっていて、今でも潜在的な危険性を持っています。
まだAさんが実装しているうちは問題ないかもしれませんが、第三者がこの部分を改修したときにバグを起さない保証はないでしょう。
getterで取得した値を使ってビジネスロジックを書いてしまっている
if (5 <= $itemOrder->getQuantity() && 100 < $itemOrder->getUnitPrice()) {
$totalPrice = $itemOrder->getUnitPrice() * $itemOrder->getQuantity();
if (3000 <= $totalPrice) {
$totalPrice = $totalPrice - 300;
}
getterは便利で、Aさんはgetterで取得した値を元に判定を行ったり、計算したりしています。
しかし、結果的に「追加の関数」のときにこの処理と同じ処理を書くことになってしまいました。
分岐・加工・計算というのは一般的にはビジネスロジックです。
このビジネスロジックを書く場所が分散されてしまっています。
後日このビジネスロジックの仕様を確認したいときに、おそらくcalc_total_price
関数とis_over
関数の2つを追う必要に迫られるでしょう。ビジネスロジックの可読性を下げています。
また、同じような追加機能の要望が来た時にもやはり同じようにコピペで対応するしかなくなると思います。ビジネスの再利用性も下げていることが分かります。
この分岐・計算はよく見るとItemOrder
クラスのインスタンス変数だけでも可能であることに気付きます。
calc_total_price
関数に書く必要はなく、ItemOrder
クラスにこれらのビジネスロジックを書いておけば良かったのです。
なぜItemOrder
クラスにビジネスロジックを書くべきかというと、ビジネスロジックのインプットとなるデータである単価、数量はItemOrder
クラスが持っているからです。
ItemOrder
クラスがビジネスロジックを持つことにより、データが使われるビジネスロジックがどこなのか、ビジネスロジックのインプットとなるデータはどれなのかが両方とも追いやすくなります。
calc_total_price
関数にビジネスロジック、ItemOrder
クラスにデータ、というように分離しているわけでもないので、再利用性も高まります。
つまり、データとビジネスロジックは密結合にしておくべきだったのに、getterによりビジネスロジックをデータを持っているオブジェクトの外へと流出させてしまったのが問題でした[8]。
ビジネスロジックを書いている場所が分散してしまったことで、ビジネスロジックの可読性と再利用性を下げています。
分かりやすいサインとして、getterで取得した値を分岐・加工・計算に使っていたらアンチパターンのサイン、と覚えておくと良いと思います[9]。
一方、逆にgetterを使わざるを得ないのが以下の部分です。
$params = [
'unit_price' => $itemOrder->getUnitPrice(),
'quantity' => $itemOrder->getQuantity(),
'ordered_at' => $itemOrder->getOrderedAt()->getTimestamp(),
];
// アイテムオーダーをDBとかに保存する処理が書かれてるとします
DBに保存するときにはinteger, stringなどのプリミティブな値として取り出し、出力する必要が出てきます。
例はDBですが、それ以外でもMVCのViewの部分、APIリクエスト・レスポンスの値など、いわゆるインターフェース層にあたる部分に対しては同様に出力する必要が出てくると思います。
getterは完全になくすことは出来ません。
よって、getterを生やした上で、レビュー等でビジネスロジックをオブジェクト外に流出させていないかを指摘し合うという運用でカバーする方針になるかと思います。
設計の改善
ここからは浮かび上がった2つの問題を改善するリファクタリングをしていきます。
setterの根絶
setterを根絶する方法は、そもそもオブジェクト生成(コンストラクタ)の段階でオブジェクトとして完成させることです。
「ItemOrder
クラスとして生成できた = オブジェクトとして正しい状態である」というルールにします。
具体的には以下の実装になります。
<?php
function calc_total_price(int $unitPrice, int $quantity): int
{
- $itemOrder = new ItemOrder($unitPrice, $quantity, new DateTimeImmutable());
- if (5 <= $itemOrder->getQuantity() && 100 < $itemOrder->getUnitPrice()) {
- $discountedPrice = $itemOrder->getUnitPrice() - 20;
- $itemOrder->setUnitPrice($discountedPrice);
- }
-
- if (!$itemOrder->validates()) {
+ try {
+ $itemOrder = new ItemOrder($unitPrice, $quantity, new DateTimeImmutable());
+ } catch (Exception $e) {
+ return 0;
+ }
$itemOrderRepository = new ItemOrderRepository();
if (!$itemOrderRepository->persist($itemOrder)) {
return 0;
}
$totalPrice = $itemOrder->getUnitPrice() * $itemOrder->getQuantity();
if (3000 <= $totalPrice) {
$totalPrice = $totalPrice - 300;
}
return $totalPrice;
}
(中略)
class ItemOrder
{
private int $unitPrice;
private int $quantity;
private DateTimeImmutable $orderedAt;
public function __construct(int $unitPrice, int $quantity, DateTimeImmutable $orderedAt)
{
+ if (5 <= $quantity && 100 < $unitPrice) {
+ $unitPrice = $unitPrice - 20;
+ }
+
+ if ($unitPrice <= 0) {
+ throw new Exception('invalid unit price');
+ }
+
+ if ($quantity <= 0) {
+ throw new Exception('invalid quantity');
+ }
+
$this->unitPrice = $unitPrice;
$this->quantity = $quantity;
$this->orderedAt = $orderedAt;
}
public function getUnitPrice(): int
{
return $this->unitPrice;
}
public function getQuantity(): int
{
return $this->quantity;
}
public function getOrderedAt(): DateTimeImmutable
{
return $this->orderedAt;
}
-
- public function validates(): bool
- {
- return 0 < $this->unitPrice && 0 < $this->quantity;
- }
-
- public function setUnitPrice(int $unitPrice)
- {
- $this->unitPrice = $unitPrice;
- }
}
(中略)
実装について簡単に説明します。
- オブジェクト生成時の引数がオブジェクトとして正しくない場合は例外を投げ、そもそもオブジェクト生成ができないようにする[10]
- 単価値引きはそもそもコンストラクタ時で完結させ、後でsetする必要がないようにする[11]
- オブジェクト生成ができなかったときのハンドリングは使用側に任せる(この場合
catch
してreturn
させています)
この実装のメリットとして、以下の3つがあります。
- setterによってオブジェクトが破壊される恐れがなくなった。
- バリデーションメソッドが不要になった。正確にはオブジェクト生成ができているというのがバリデーションに通ってることを意味するようになった。
-
calc_total_price
関数の処理が減った。
getterによるビジネスロジックの流出を阻止
次にgetterを使ってビジネスロジックをオブジェクト外に流出させてしまっている箇所を改善します。
こちらは簡単で、ビジネスロジックをオブジェクトに持たせるだけで良いです。
<?php
function calc_total_price(int $unitPrice, int $quantity): int
{
try {
$itemOrder = new ItemOrder($unitPrice, $quantity, new DateTimeImmutable());
} catch (Exception $e) {
return 0;
}
$itemOrderRepository = new ItemOrderRepository();
if (!$itemOrderRepository->persist($itemOrder)) {
return 0;
}
- $totalPrice = $itemOrder->getUnitPrice() * $itemOrder->getQuantity();
- if (3000 <= $totalPrice) {
- $totalPrice = $totalPrice - 300;
- }
-
- return $totalPrice;
+ return $itemOrder->totalPrice();
}
(中略)
class ItemOrder
{
private int $unitPrice;
private int $quantity;
private DateTimeImmutable $orderedAt;
public function __construct(int $unitPrice, int $quantity, DateTimeImmutable $orderedAt)
{
- if (5 <= $quantity && 100 < $unitPrice) {
+ if (self::isUnitPriceDiscounted($unitPrice, $quantity)) {
$unitPrice = $unitPrice - 20;
}
if ($unitPrice <= 0) {
throw new Exception('invalid unit price');
}
if ($quantity <= 0) {
throw new Exception('invalid quantity');
}
$this->unitPrice = $unitPrice;
$this->quantity = $quantity;
$this->orderedAt = $orderedAt;
}
public function getUnitPrice(): int
{
return $this->unitPrice;
}
public function getQuantity(): int
{
return $this->quantity;
}
public function getOrderedAt(): DateTimeImmutable
{
return $this->orderedAt;
}
+
+ public function totalPrice(): int
+ {
+ $totalPrice = $this->unitPrice * $this->quantity;
+ if ($this->isTotalPriceDiscounted()) {
+ $totalPrice = $totalPrice - 300;
+ }
+
+ return $totalPrice;
+ }
+
+ private function isTotalPriceDiscounted(): bool
+ {
+ return 3000 <= $this->unitPrice * $this->quantity;
+ }
+
+ private static function isUnitPriceDiscounted(int $quantity, int $unitPrice): bool
+ {
+ return 5 <= $quantity && 100 < $unitPrice;
+ }
}
(中略)
最終実装
<?php
function calc_total_price(int $unitPrice, int $quantity): int
{
try {
$itemOrder = new ItemOrder($unitPrice, $quantity, new DateTimeImmutable());
} catch (Exception $e) {
return 0;
}
$itemOrderRepository = new ItemOrderRepository();
if (!$itemOrderRepository->persist($itemOrder)) {
return 0;
}
return $itemOrder->totalPrice();
}
// 仕様1: 単価*個数が返ってくること
assert(calc_total_price(200, 4) === 800);
// 仕様2: 単価がマイナスではエラーとなること
assert(calc_total_price(-500, 4) === 0);
// 仕様3: 個数が5個以上なら単価は20円引きとなること
assert(calc_total_price(200, 5) === 900);
// バグ: 合計額がマイナスになる!?
echo calc_total_price(15, 10) . PHP_EOL;
// 仕様4: 単価が100円以下のときには単価の値引きをしないこと
assert(calc_total_price(10, 5) === 50);
// 仕様5: 合計額が3000円以上のときには合計額は300円引きとなること
assert(calc_total_price(1000, 4) === 3700);
echo 'all green' . PHP_EOL;
class ItemOrder
{
private int $unitPrice;
private int $quantity;
private DateTimeImmutable $orderedAt;
public function __construct(int $unitPrice, int $quantity, DateTimeImmutable $orderedAt)
{
if (self::isUnitPriceDiscounted($unitPrice, $quantity)) {
$unitPrice = $unitPrice - 20;
}
if ($unitPrice <= 0) {
throw new Exception('invalid unit price');
}
if ($quantity <= 0) {
throw new Exception('invalid quantity');
}
$this->unitPrice = $unitPrice;
$this->quantity = $quantity;
$this->orderedAt = $orderedAt;
}
public function getUnitPrice(): int
{
return $this->unitPrice;
}
public function getQuantity(): int
{
return $this->quantity;
}
public function getOrderedAt(): DateTimeImmutable
{
return $this->orderedAt;
}
public function totalPrice(): int
{
$totalPrice = $this->unitPrice * $this->quantity;
if ($this->isTotalPriceDiscounted()) {
$totalPrice = $totalPrice - 300;
}
return $totalPrice;
}
private function isTotalPriceDiscounted(): bool
{
return 3000 <= $this->unitPrice * $this->quantity;
}
private static function isUnitPriceDiscounted(int $quantity, int $unitPrice): bool
{
return 5 <= $quantity && 100 < $unitPrice;
}
}
class ItemOrderRepository
{
public function persist(ItemOrder $itemOrder): bool
{
$params = [
'unit_price' => $itemOrder->getUnitPrice(),
'quantity' => $itemOrder->getQuantity(),
'ordered_at' => $itemOrder->getOrderedAt()->getTimestamp(),
];
// アイテムオーダーをDBとかに保存する処理が書かれてるとします
return true;
}
}
ほぼ不要と思いますが、実装について簡単に説明します。
- 合計額の計算のビジネスロジックをgetterを使ってオブジェクト外に流出させずに、
ItemOrder
クラスに持たせた。 - ifの分岐をprivateメソッドとして命名することで、どういうビジネスロジックなのかをコードとして明文化した[12]。
これによってビジネスロジックの可読性・再利用性が高まっているはずです。
追加の関数により可読性・再利用性が向上したことの確認
上のリファクタリングによって、追加の関数がどうなったかを確認します。
<?php
require_once 'calc.php';
function is_over(int $unitPrice, int $quantity): bool
{
try {
$itemOrder = new ItemOrder($unitPrice, $quantity, new DateTimeImmutable());
} catch (Exception $e) {
return false;
}
return 10000 <= $itemOrder->totalPrice();
}
// 仕様1: 合計額が10000円未満 (1000 - 20) * 10 - 300 = 9500 < 10000
assert(is_over(1000, 10) === false);
// 仕様2: 合計額が10000円 (1050 - 20) * 10 - 300 >= 10000
assert(is_over(1050, 10) === true);
echo 'all green' . PHP_EOL;
php is_over.php
all green
元の実装と比べれば、以下のことが分かると思います。
- コンストラクタとメソッド呼び出しの2手のみの手続きで済んでいる。
- あまり
ItemOrder
クラスの中身を理解せずとも処理を書けている[13]。 - DRYとなり、ビジネスロジックの再利用性が高まっている。
まとめ
- getter/setterの善悪は一律で判断できない。
- setterはバグの温床になりうるので悪。
- setterを根絶するにはオブジェクト生成の段階でオブジェクトとして完成させる。
- getterは悪とされるパターンとそうでないパターンが存在する。
- インターフェース層のためにgetterが必要となる。
- getterによりビジネスロジックをオブジェクトの外へと流出させてしまうのは悪。
補足
- オブジェクト指向をちゃんと勉強したい場合は「現場で役立つシステム設計の原則」を読めば間違いないです。全人類、この本を読みましょう。
- 例えばRailsのActiveRecordはこの記事の思想とは大きく反していると思います。やっぱり現場によるので、柔軟に対応しましょう。
- ドヤ顔で解説記事を書きましたが、筆者もオブジェクト指向にそこまで自信があるわけではないです[14]。コメント・マサカリを歓迎しております。
-
設計によってはgetterを使わなくても良い設計にできることもあるようですが、入門者はあまり気にしなくて良いと思います。 ↩︎
-
PHPUnitはこの記事の趣旨に対してオーバーエンジリアリングです。 ↩︎
-
モデリングに関してはノーコメントでお願いします。 ↩︎
-
リポジトリや現在時刻のDIでの実装はこの記事の趣旨に対してオーバーエンジリアリングです。 ↩︎
-
もちろんビジネスサイドも悪いかもしれませんが、こういう仕様の抜け道はエンジニアが気付きやすいので、気付いて指摘するべきですね。 ↩︎
-
そんな特定のユースケースなんてある??っていうツッコミは無しでお願いします。 ↩︎
-
唐突な再宣言ですが、これがないとgetter/setter以外の設計に対する大量のマサカリを受けることになる、と供述しています。 ↩︎
-
ここまでドヤ顔で解説していますが、「現場で役立つシステム設計の原則」に同じことが書いてあります。 ↩︎
-
アンチパターンのサインというだけで、絶対にNGというわけではありません。結局はどこの責務なのかという話になります。 ↩︎
-
本来は独自例外クラスを定義すべきですが、この記事の趣旨に対してオーバーエンジリアリングです。 ↩︎
-
アイテムオーダーのクラスと、単価が値引きされた結果のクラスは、ちゃんと分けるべきって?僕もそう思います。 ↩︎
-
数字の定数化は面倒だった。いやこの記事の趣旨に対してオーバーエンジリアリングですよ(?)。 ↩︎
-
でもそれってマジでクールなことでさ、オレは知る必要がないのさ ↩︎
-
この記事書き終わった後にTwitterのTL見たら、偶然にもオブジェクト指向の話がバチバチに盛り上がっていて怖くなった、と供述しています。 ↩︎
Discussion
setter/getterの議論の範囲で言えば、getterは悪なのです。
これはレイヤーをまたがる際の共通的な悩みですね。。ですが、オブジェクトをレイヤ超えて持ち出すのがそもそも望ましくなかったりします。
最近だとクリーンアーキテクチャという本が、そのまんまの解答を提示していて、依存性の逆転を使うことで、レイヤーを超えたオブジェクトの受け渡しが可能になります。
レイヤーを超える必要がない場合、提示されたユースケースが消えるためgetterは悪となります。
例えば、ItemOrderに下記のような拡張をします。
するとこんな風に使えます。
ItemOrder には、getterが全く必要ありません。ただし、OrderInfo が結局getterの代わりにそのままアクセスしてるだけじゃん。となりますね。これは、構造体やDTOと呼ばれる情報を保持するオブジェクトです。
ItemOrderの中身ではなく、描画するためのOrderの情報が格納されているだけになります。
ここでsetter/getterの議論の難しさが登場します。
オブジェクトの責務に備わないsetter/getter(限らずメソッド全般)は悪だとなります。
そのため、ロジックを実行するためのオブジェクトにstter/getterは必要ありません。
さらに、setter/getterではなく、プロパティアクセスという方法があります。
情報保持オブジェクトは、保持役のためプロパティアクセスでも安全ならでそれでよいのです。
プロパティ系の構文を持っていない言語では、文法的な統一性やReadOnlyのため、プロパティアクセスのような意図でgetterを挟むことが良くあります。
さらに、ややこしいのが、OOPにもMutableな思想が根付きました。
たとえ情報保持オブジェクトでも、setterを持つ必要性がなくなってきた。
そのため、情報保持オブジェクトは、取得のみプロパティアクセスできれば良い。となり始めました。
さらに、さらにややこしいのが、実用面的なトレードオフからActiveRecordのような意図的に責務を兼任させるオブジェクト指向のパターンもある。レイヤーをまたがる場合のオーバヘッドをトレードオフとするわけですね。
結果として、ItemOrder にgetterがはえてても良いと考えてもいいんじゃね。となります。
逆に言えば、言語仕様的な制限やトレードオフ的な部分も全部クリアして、超理想論にまで突き詰めた場合、オブジェクト指向のカプセル化の原則に違反するsetter/getterは、悪になります。
setter/getterだけで、ここまで実はオブジェクト指向の真髄に迫れるのが面白く、思わずコメントさせて頂きましたw
ありがとうございます!
なるほど、別レイヤからは単にgetterを使うのではなく、データ描写だけされたDTOを使うってことですね。
確かにその方法でgetterも根絶できそうです、いやめちゃくちゃ勉強になりました。
そもそもプロパティアクセスをしたいならgetterという選択肢しかない言語もあるわけですね。
なのでDTOでも「getter」をはやさなくちゃいけず、それがgetter許容論に曲解されてしまうと。
これはかなりややこしいですよね。ActiveRecordだとむしろ本記事のAさんみたいに、setterで詰め込んでDB保存前にvalidateメソッドさえ走らせれば良い、という設計になっているので。
Rails出身の方がDDD・オブジェクト指向を学び直したときになかなか苦労しそうだと思ってます。
そうですね、少なくとも純粋なドメインを表現するオブジェクトには、前述の方法でgetterは不要そうです。