Мы, PHP-разработчики, горды тем, что пишем на ООП-языке (можно легко здесь заменить PHP на C#, Java или другой ООП-язык). Каждая вакансия содержит требования про знание ООП. В каждом собеседовании спрашивают что-нибудь про SOLID или трех "китов" ООП. Но когда дело доходит до дела — мы получаем просто классы, наполненные процедурами. ООП проявляется редко, обычно в коде библиотек.
Обычное веб-приложение — это классы ORM-сущностей, которые содержат данные из строки в базе данных и контроллеры(или сервисы — неважно), содержащие процедуры работы с этими данными. Объектно-ориентированное программирование — оно про объекты, которые владеют собственными данными, а не предоставляют их для обработки другому коду. Отличная иллюстрация этого — вопрос, который был задан в одном чате: "Как я могу улучшить этот код?"
private function getWorkingTimeIntervals(CarbonPeriod $businessDaysPeriod, array $timeRanges): array
{
$workingTimeIntervals = [];
foreach ($businessDaysPeriod as $date) {
foreach ($timeRanges as $time) {
$workingTimeIntervals[] = [
'start' => Carbon::create($date->format('Y-m-d') . ' ' . $time['start']),
'end' => Carbon::create($date->format('Y-m-d') . ' ' . $time['end'])
];
}
}
return $workingTimeIntervals;
}
/**
* Удалить события из расписания
*
* @param array $workingTimeIntervals
* @param array $events
* @return array
*/
private function removeEventsFromWorkingTime(array $workingTimeIntervals, array $events): array
{
foreach ($workingTimeIntervals as $n => &$interval) {
foreach ($events as $event) {
$period = CarbonPeriod::create($interval['start'], $interval['end']);
if ($period->overlaps($event['start_date'], $event['end_date'])) {
if ($interval['start'] <= $event['start_date'] && $interval['end'] <= $event['end_date']) {
$interval['end'] = $event['start_date'];
} elseif ($interval['start'] >= $event['start_date'] && $interval['end'] >= $event['end_date']) {
$interval['start'] = $event['end_date'];
} elseif ($interval['start'] <= $event['start_date'] && $interval['end'] >= $event['end_date']) {
$interval['start'] = $event['start_date'];
$interval['end'] = $event['end_date'];
} else {
unset($workingTimeIntervals[$n]);
}
}
}
}
return $workingTimeIntervals;
}
Этот код работает с временными интервалами. Метод удаляет множество одних интервалов(событий) из множества других(расписания), чтобы получить свободное время в расписании. Как видите, вся работа здесь вертится вокруг структуры из двух дат. И стиль работы с данными весьма распространённый — некая структура данных(здесь массив) и некий сторонний код, который с ней работает. Организуя код так, легко дублировать его, когда схожая логика понадобится в другом месте. Давайте попробуем сконцентрировать данные и код, с ними работающий, в одном месте.
Каждый раз начиная рефакторинг я проверяю наличие unit-тестов для данного кода. Рефакторинг без тестов может быть весьма болезненным. Код из примера их не имеет и его структура не даёт возможности быстро их написать (методы приватные). Поэтому будем аккуратно проверять логику после рефакторинга.
class Interval
{
// Типизированные поля из PHP 7.4
public DateTimeImmutable $start;
public DateTimeImmutable $end;
}
Класс DateTimeImmutable был использован как более удобная альтернатива для работы с датами.
Первое требование к этому классу — начальная дата не должна быть больше конечной.
Намного удобнее когда объект, не удовлетворяющий требованиям, даже не может быть создан, поэтому конструктор — лучшее место для этой проверки.
Я почти всегда начинаю описывать требования к классу через unit-тесты. Позже будет понятно почему. Начнем с обычного конструктора:
class Interval
{
public DateTimeImmutable $start;
public DateTimeImmutable $end;
public function __construct(DateTimeImmutable $start, DateTimeImmutable $end)
{
$this->start = $start;
$this->end = $end;
}
}
Теперь PHPUnit-тесты для нашего требования:
use App\Interval;
use PHPUnit\Framework\TestCase;
class IntervalTest extends TestCase
{
private DateTimeImmutable $today;
private DateTimeImmutable $yesterday;
private DateTimeImmutable $tomorrow;
protected function setUp(): void
{
$this->today = new DateTimeImmutable();
$this->yesterday = $this->today->add(\DateInterval::createFromDateString("-1 day"));
$this->tomorrow = $this->today->add(\DateInterval::createFromDateString("1 day"));
parent::setUp();
}
public function testValidDates()
{
$interval = new Interval($this->yesterday, $this->today);
$this->assertEquals($this->yesterday, $interval->start);
$this->assertEquals($this->today, $interval->end);
}
public function testInvalidDates()
{
$this->expectException(\InvalidArgumentException::class);
new Interval($this->today, $this->yesterday);
}
}
Объекты дат для сегодняшнего, вчерашнего и завтрашнего дня помогут читабельности тестов. Первый тест, testValidDates
, просто проверяет обычное создание интервала. Второй, testInvalidDates
, пытается создать неправильный интервал и ожидает эксепшен. Первый тест пройдет нормально, второй свалится с ошибкой:
Failed asserting that exception of type "InvalidArgumentException" is thrown.
Теперь реализуем эту проверку:
class Interval
{
public DateTimeImmutable $start;
public DateTimeImmutable $end;
public function __construct(DateTimeImmutable $start, DateTimeImmutable $end)
{
if ($start > $end) {
throw new \InvalidArgumentException("Invalid date interval");
}
$this->start = $start;
$this->end = $end;
}
}
Теперь оба тесты будут зелеными. Спасибо современной системе типов в PHP, мы не обязаны проверять null и другие значения. Это сильно сокращает объем тестов. Однако, каждый раз, когда разработчик пишет тесты он обязан проверить краевые значения. Очевидным краевым значением для объекта Interval будут одинаковые даты начала и конца. Возможно такое или нет? Вот как unit-тесты помогают создавать хороший дизайн. Они задают хорошие вопросы еще до того, как мы написали настоящий код. До того как разработчик сам задаст их себе. Давайте решим, что пустые интервалы возможны, но добавим новый метод isEmpty
к этому классу.
class Interval
{
public DateTimeImmutable $start;
public DateTimeImmutable $end;
public function __construct(DateTimeImmutable $start, DateTimeImmutable $end)
{
if ($start > $end) {
throw new \InvalidArgumentException("Invalid date interval");
}
$this->start = $start;
$this->end = $end;
}
public function isEmpty(): bool
{
return $this->start->getTimestamp() == $this->end->getTimestamp();
}
}
class IntervalTest extends TestCase
{
//...
public function testNonEmpty()
{
$interval = new Interval($this->yesterday, $this->today);
$this->assertFalse($interval->isEmpty());
}
public function testEmpty()
{
$interval = new Interval($this->today, $this->today);
$this->assertTrue($interval->isEmpty());
}
}
Мы построили базовый класс для интервала дат. Он может быть использован вместо массивов ['start'=>,'end'=>]
, но большого смысла в этом нет. Давайте добавим знания об интервале дат в этот объект! Начальное задание было о создании списка свободных интервалов, на основе расписания и занятых слотов. Первый метод создавал интервалы, например:
какой-то день 08:00 - 12:00
какой-то день 13:00 - 17:00
следующий день 08:00 - 12:00
следующий день 13:00 - 17:00
...
Второй метод удалял занятые слоты из расписания, оставляя лишь свободные:
Занятые слоты:
какой-то день 08:00 - 09:00
какой-то день 16:00 - 17:00
следующий день 13:00 - 17:00
Результат:
какой-то день 09:00 - 12:00
какой-то день 13:00 - 16:00
следующий день 08:00 - 12:00
...
Я хочу переместить эту логику в класс Interval:
$period = CarbonPeriod::create($interval['start'], $interval['end']);
if ($period->overlaps($event['start_date'], $event['end_date'])) {
if ($interval['start'] <= $event['start_date'] && $interval['end'] <= $event['end_date']) {
$interval['end'] = $event['start_date'];
} elseif ($interval['start'] >= $event['start_date'] && $interval['end'] >= $event['end_date']) {
$interval['start'] = $event['end_date'];
} elseif ($interval['start'] <= $event['start_date'] && $interval['end'] >= $event['end_date']) {
$interval['start'] = $event['start_date'];
$interval['end'] = $event['end_date'];
} else {
unset($workingTimeIntervals[$n]);
}
}
Новый метод класса Interval: remove(Interval $other)
будет изменять объект, удаляя из него переданный интервал. Код станет намного чище:
private function removeEventsFromWorkingTime($workingTimeIntervals, $events): array
{
foreach ($workingTimeIntervals as $n => $interval) {
foreach ($events as $event) {
$interval->remove($event);
if ($interval->isEmpty()) {
unset($workingTimeIntervals[$n]);
}
}
}
return $workingTimeIntervals;
}
Отлично. Время реализовать новый метод. И начнём, очевидно, с тестов! Когда разработчик начинает разрабатывать с тестов, он обязан подумать о требованиях к классу и методам и подумать хорошо. Это полезно. Проанализируем требования, рассмотрев возможные случаи.
Интервал не должен быть изменён, если $other
не касается его.
class IntervalRemoveTest extends TestCase
{
private DateTimeImmutable $minus10Days;
private DateTimeImmutable $today;
private DateTimeImmutable $yesterday;
private DateTimeImmutable $tomorrow;
private DateTimeImmutable $plus10Days;
protected function setUp(): void
{
$this->today = new DateTimeImmutable();
$this->yesterday = $this->today->sub(\DateInterval::createFromDateString("1 day"));
$this->tomorrow = $this->today->add(\DateInterval::createFromDateString("1 day"));
$this->minus10Days = $this->today->sub(\DateInterval::createFromDateString("10 day"));
$this->plus10Days = $this->today->add(\DateInterval::createFromDateString("10 day"));
parent::setUp();
}
public function testDifferent()
{
$interval = new Interval($this->minus10Days, $this->yesterday);
$interval->remove(new Interval($this->tomorrow, $this->plus10Days));
$this->assertEquals($this->minus10Days, $interval->start);
$this->assertEquals($this->yesterday, $interval->end);
}
}
Интервал, полностью покрытый переданным интервалом, должен стать пустым.
class IntervalRemoveTest extends TestCase
{
public function testFullyCovered()
{
$interval = new Interval($this->yesterday, $this->tomorrow);
$interval->remove(new Interval($this->minus10Days, $this->plus10Days));
$this->assertTrue($interval->isEmpty());
}
public function testFullyCoveredWithCommonStart()
{
$interval = new Interval($this->yesterday, $this->tomorrow);
$interval->remove(new Interval($this->yesterday, $this->plus10Days));
$this->assertTrue($interval->isEmpty());
}
// and testFullyCoveredWithCommonEnd()
}
Следующий случай, когда переданный интервал частично перекрывает текущий:
Что если переданный интервал располагается внутри текущего?
Эмммм?! Наш интервал разделился на два! Весь дизайн нашего метода remove
оказался неверным! Кстати, и изначальный код тоже содержит ошибку в этом месте:
} elseif ($interval['start'] <= $event['start_date'] && $interval['end'] >= $event['end_date']) {
$interval['start'] = $event['start_date'];
$interval['end'] = $event['end_date'];
Но обнаружена она будет сильно позже и хорошо если не пользователями приложения.
Мы даже не начали писать сам код, но уже нашли большую проблему в дизайне. Да, этот пример весьма прост и многие разработчики нашли бы проблему и без тестов, но для более сложных случаев найти проблему обычными умозаключениями будет весьма непросто. Вот почему тесты полезны. Они помогают писать код, часто избегая серьезных просчетов в дизайне. Некоторые разработчики говорят, что не любят писать тесты, потому что с ними код пишется медленней. Для простых тестов, вроде тех, которые мы написали вначале, это верно. Но для ситуаций посложнее, код с тестами часто написать бывает быстрее, чем без них! Тесты задают хорошие и весьма точные вопросы про дизайн вашего кода. Разработчик обязан создавать хороший дизайн.
Одним из возможных решений является создание нового класса — IntervalCollection, представляющего собой множество интервалов и операции над ними:
class Interval
{
public DateTimeImmutable $start;
public DateTimeImmutable $end;
public function __construct(DateTimeImmutable $start,
DateTimeImmutable $end)
{
if ($start > $end) {
throw new \InvalidArgumentException(
"Invalid date interval");
}
$this->start = $start;
$this->end = $end;
}
public function isEmpty(): bool
{
return $this->start === $this->end;
}
/**
* @param Interval $other
* @return Interval[]
*/
public function remove(Interval $other)
{
if ($this->start >= $other->end
|| $this->end <= $other->start) return [$this];
if ($this->start >= $other->start
&& $this->end <= $other->end) return [];
if ($this->start < $other->start
&& $this->end > $other->end) return [
new Interval($this->start, $other->start),
new Interval($other->end, $this->end),
];
if ($this->start === $other->start) {
return [new Interval($other->end, $this->end)];
}
return [new Interval($this->start, $other->start)];
}
}
/** @mixin Interval[] */
class IntervalCollection extends \ArrayIterator
{
public function diff(IntervalCollection $other)
: IntervalCollection
{
/** @var Interval[] $items */
$items = $this->getArrayCopy();
foreach ($other as $interval) {
$newItems = [];
foreach ($items as $ourInterval) {
array_push($newItems,
...$ourInterval->remove($interval));
}
$items = $newItems;
}
return new self($items);
}
}
Класс IntervalCollection — это другой пример концентрации логики рядом с данными, которыми она оперирует. Не просто массив с объектами Interval, который обрабатывается некоторыми сторонними функциями, а целый класс с правильным именем и покрытый тестами.
Полный исходный код с тестами можно найти здесь — https://github.com/adelf/intervals-example. Текущее решение не очень оптимальное с точки зрения производительности, но логика его спрятана в методе IntervalCollection::diff
и хорошо покрыта тестами. Если другой разработчик захочет оптимизировать его, он сможет это сделать без всякого страха. Любую ошибку в логике тесты поймают немедленно. Это второе преимущество unit-тестов.
Организация вашего кода объектами, которые по-настоящему владеют своими собственными данными помогают сильно уменьшить coupling (связанность или зацепление), что очень важно в больших проектах. Следующим этапом концентрации данных и кода будет переделка модификаторов доступа полей на private:
class Interval
{
private DateTimeImmutable $start;
private DateTimeImmutable $end;
// methods
}
Это возможно сделать добавлением методов в стиле print()
, которые помогут нам вытащить данные об интервале в нужном формате, но полностью закроет возможность работать с данными интервала извне. Но это уже точно тема для другой статьи.
skyeff
Интересно, автор оригинала это сам придумал или ему кто-то подсказал. Инкапсуляция — это только часть ООП, а не все ООП. Хотелось бы узнать ответы автора оригинала на пару вопросов. Есть Клиент, есть Заказ и есть бизнес функция Рассчитать скидку.
Вопрос 1: куда поместить логику расчета скидки?
1. В класс клиента.
2. В класс заказа.
3. В отдельный сервис.
Вопрос 2. Кто в итоге будет «владеть» всем набором данных?
1. Класс клиента.
2. Класс заказа.
3. Сервис.
4. Никто.
В целом я бы переформулировал: если для выполнения некой логики достаточно знать состояние одной сущности, лучше поместить эту логику непосредственно в объект этой сущности.
Но предложенный в дальнейшем пример рефакторинга вполне годный.
Adelf Автор
Автор оригинала немного неточно выразился. Разумеется, можно предоставить свои данные, самому передав их в другой объект или метод.
Кроме того, не стоит быть догматичным. Любое приложение всегда будет немного процедурным, немного ООПшным, немного функциональным. Только пропорции немного меняются в зависимости от предпочтений разработчиков.
В приведённом примере логику можно сунуть туда, где ей удобно. В VO цены, сущности заказа или даже создать DiscountCalculator объект, передав ему всё необходимое. Это уже зависит от построенной модели. Вот только сервис — это уже что-то совсем процедурное. Сервис захочет залезть в сущности в поисках необходимой ему инфы, что не очень, но это уже догматизм, см. предыдущий абзац.
skyeff
Нет, у сервиса так же может быть состояние, в отличие от процедуры. Только состояние это будет называться конфигурация сервиса. Например:
— у кого узнать план подписки клиента, вдруг, у него премиум подписка;
— где взять актуальные скидочные программы, например, за каждые 3 позиции товара А 20% скидки на одну позицию товара Б?
Один сервис явно все это не сможет объять. А уж если эту логику засунуть внутрь сущности или VO, он точно лопнет.
Т.е. когда всей полнотой информации не владеет ни одна из сущностей, участвующих в бизнес процессе, нужно вводить некий координатор — сервис. И это не будет процедурным/функциональным подходом — это будет нормальным распределением обязанностей.
nemavasi
А доставка связана с заказом, нет заказа — нет доставки? Т.е. всю логику доставки вносим в заказ? А если у нас скидка определяется только планом клиента? А если скидка определяется историей заказов? А если скидка определяется способом доставки? Все равно все в заказ, так как нет заказа — нет скидки?
Еще раз: в данной ситуации полнотой информации, не «большей частью информации», а всей полнотой информации не владеет никто. Значит за выполнение логики должен отвечать объект более высокого уровня, который будет в состоянии получить требуемую информацию от каждой сущности вовлеченной в процесс.
Adelf Автор
Тут понятно. Тот самый DiscountCalculator, который я предлагал. Просто у меня в голове сервисы — существа stateless, поэтому я немного не так интерпретировал сообщение. Все эти объекты-координаторы вещь весьма спорная, но я бы тут этот спор не хотел разводить. Статья о том, что иногда надо взглянуть на свои данные и код по-другому.
nemavasi
У Егора Бугаенко есть отдельная статья в книге про классы, название которых оканчивается на -or и -er
Adelf Автор
Когда я писал этот ответ, я думал как раз про то, что пропагандирует Егор. Он пропагандирует тот самый чистый ООП, без процедурщины вообще, и этот подход хорош в лабораторных условиях, но в нормальной жизни не применим.
nemavasi
Приемлим — только надо больше думать вначале.
kai3341
Конечно приемлим. Здравый смысл? Не, не слышал.
Anton_Zh
То что пишет Mr. Бугаенко, надо фильтровать при прочтении. Неопытному лучше поберечься. Это больше по именования, а не про распределение обязанностей. В GRASP есть шаблон PureFabrication, он как раз про сервисы.
nemavasi
Мы говорим про скидку. Скидка на что? — на заказ. При этом никто не мешает методу расчета в составе заказа обращаться к другим объектам — например к Журналу заказов, к объекту Доставка и т.п. Давайте не будем устраивать из кода кашу. Никто не говорит что все данные должны быть внутри объекта. Надо все-таки отличать концепцию «объект — это данные и методы работы с этими данными» от «объект — это сущность реального мира». Стараюсь где только можно придерживаться второй концепции.
skyeff
Скидка для кого? — для клиента.
Где мы разместим метод? Кто будет обращаться к данным пользователя, к настройкам доставки, к истории заказа? Класс Заказ? Курсы из справочника курсов валют тоже класс Заказ должен получать? Погоду на месяц? Все это должны делать внешние сервисы. Класс Заказ вообще не должен знать ничего об истории заказов, доставке (если это не отдельная позиция в строке заказа), курсах и т.п. Он может иметь ссылку на сущность Клиент, для удобства доступа к данным, но ему уж точно не надо знать о тарифных планах клиентов.
VolCh
В реальной жизни скидку вам будет считать человек, часто именуемый менеджер по продажам, у которого будет доступ и к данным заказа, и к данным клиента, включая историю его заказов.
Причём собственно расчёт он может делегировать человеку, у которого основная обязанность считать. Сюрприз — таких людей называли калькуляторами.
qrKot
У вас буковка S из SOLID отвалилась…
nemavasi
Экземпляр Заказ. К нему прикреплен экземпляр Цена, к которому прикреплен экземпляр Скидка, внутри которого метод расчета (возможно ленивый) непосредственного значения скидки. Где именно отвалилась S?
А вот если методы существуют отдельно в разных утилитных классах или сервисах — тогда это уже не объекты в полном смысле, а структуры данных. И получаем анемичную модель, со всеми вытекающими. Уж тогда лучше переходить на функциональное программирование
VolCh
А откуда метод расчёта скидки возьмёт историю заказов конкретного клиента или ещё какие его данные?
qrKot
Не совсем понимаю, какого рода связь вы подразумеваете под словом «прикреплен», но ваша вышеизложенная идея «Скидка на что? — на заказ.» кажется мне, как минимум, странной и как максимум, попахивающей GodObject.
Собственно, заказ в такой терминологии будет:
1. Хранить/предоставлять доступ к списку позиций в заказе.
2. Осуществлять калькуляцию стоимости/цены заказа.
3. Высчитывать скидку.
4. Иметь еще какой-то функционал…
Адепту подхода «композиция круче наследования» во мне больно, особенно от осознания того, что S в SOLID — это Single-responsibility principle.
Собственно, размер скидки может зависеть как от суммарной суммы заказа, так и от наличия какого-то количества определенных позиций, от текущей даты/времени, положения звезд, истории клиента, наличие поштучных скидок на какие-либо позиции и т.д. и т.п.
Оно, в принципе, понятно, что метод/сервис рассчета скидок должен обладать информацией о составе заказа, это я даже не отрицаю. Но сама идея затянуть вообще все, что может понадобиться для рассчета скидки в объект заказа наряду с самой механикой рассчета меня откровенно пугает.
Заказ должен приходить на вход рассчета скидки, имхо.
Ну, собственно, не совсем анемичную. Идея валидации состава заказа на объектом «заказ» отторжения не вызывает. Отторжение вызывает собственно 2 вещи:
1. Прикрутить «динамично изменяемую модель расчета скидок» к собственно заказу (с последующей болью от рефакторинга).
2. Попытка «натянуть сову на глобус» в поисках «сущности реального мира, которой соответствует конкретно этот объект».
Дело в том, что логика рассчета скидок как таковая вполне себе достаточно крупная вещь для того, чтобы она была отдельным объектом. Если уж сильно-сильно хочется найти соотетствующую сущность, представьте себе 150-страничный документ «Регламент рассчета скидки по организации ООО Рога-и-копыта от 17.02.1986г».
Именно для этого кейса (рассчет скидки и прочие преобразования изначального заказа), имхо, достаточно интересная идея.
nemavasi
Конкретно по вашему примеру: в жизни скидка бывает связана с заказом. Нет заказа — нет и скидки. И надо стараться быть «ближе к реальности». Данные заказа однозначно понадобятся для расчета скидки. А вот данные клиента — не факт. Может быть такая ситуация, что большая часть данных для расчета окажутся как раз в классе Клиент (например, скидка покупателям старше 60 лет, плюс куча данных по его бонусам, принадлежности в разным группам и т.п.). Но все равно метод расчета скидки должен быть внутри Заказа (возможно не в самом классе Заказ, а еще в одном классе, связанным с заказом).
welovelain
Нет клиента — нет и заказа. Значит, все связанное с заказом должно быть в клиенте.
a1ex322
Все должно быть в сервисах. Наличие метода экземпляра обладает сразу несколькими недостатками: 1. npe, если объекта нет. 2. зависимость от состояния объекта. 3. Если объект ещё и наследуется, то получается просто грусть печаль, в виде попробуй отследи экземпляр какого класса сейчас выполняет логику.
nemavasi
Получится анемичная модель со всеми ее недостатками в крупных проектах
anonymous
А почему нет заказа — нет скидки?
Скидка может быть у клиента, но он может ничего не заказывать.
Вот например, мне сервис такси дал купон на следующую поездку но я больше не поеду в этом такси.
VolCh
Это всё же купон, предоставляющий право на скидку при оплате заказа. Ну, обычно они такие.
anonymous
Так же и карты скидок, они тоже такие. И где это должно жить по вашему?
VolCh
Карты скидок обычно немного другие. Часто это просто идентификатор клиента технически и при очередном заказе считается сначала скидка в деньгах для заказа, а потом новый процент скидки для следующих заказов. Некоторые продавцы даже советуют "давайте мы вот это на один чек пробъём, у вас следующая ступень скидки будет, и остальное уже с новой скидкой".
А вообще, имхо, расчёт скидки должен жить в отдельном сервисе.