метод без side effects, которому нужно возвращать результат

ivanov77

Новичок
Снова привет.
Просмотрел лекцию java ментора по чистому коду, один момент остался не совсем понятен.
Он там говорит о том что метод который что то делает не должен возвращать результат в виде статус кодов, и null в том числе, а если что то пошло не так то кидать Exception. Но все же есть случай когда вроде как надо вернуть, то делить метод на два, один проверяющий, другой так же само все равно кидающий исключение
Вот тут всего если минутку послушать.
Так что, в коде, это будет выглядеть так, и это норм?:
Код:
class ArticleService
{
    public function update(ArticleARModel $article, array $formData)
    {
        $article->setAttributes($formData->getAttributes());
        //additional actions
        //...
        $article->save();
    }

    public function checkIfExistsById($id)
    {
        return (bool) ArticleARModel::findOne($id);
    }

    public function getById($id)
    {
        if ($article = ArticleARModel::findOne(($id)) {
            return $article;
        }
        throw new DomainException('Article not found');
    }

}
Код:
class ArticleController extends \yii\web\Controller
{
    /**
     * @var ArticleService
     */
    private $service;

    public function actionUpdate($id)
    {
        if (!$this->service->checkIfExistsById($id)) {
            // что то делаем дополнительное, может редирект, но вообще то тут в основном будет:
            throw new HttpNotFoundException('Article not found');
        }
        try {
            $article = $this->service->getById($id);
        } catch (DomainException $e) {
            throw new HttpNotFoundException('Article not found');
        }
        
        $form = new ArticleForm();
        $form->load(Yii::$app->request->post());
        if ($form->validate()) {
            $this->service->update($article, $form);
            $this->redirect('article', [$article->id ]);
        }
        //...
    }

}
Тут получается 2 запроса, но он в этом не видит проблемы, видимо у них там еще кеши настроены под такое.
И два раза throw получился
А профит получается в чем - ArticleService::getById() гарантированно должно возвращать только то что запрошено, а не например еще null как статус того что не найдено.
 

fixxxer

К.О.
Партнер клуба
По примерам видно, что у тебя в голове yii.
Обсуждать "чистый код" на примере подходов, используемых в yii, бесполезно.
 

AnrDaemon

Продвинутый новичок
У тебя явно прослеживается тенденция впихнуть в один метод побольше действий. Не надо так делать.
 

ivanov77

Новичок
По примерам видно, что у тебя в голове yii.
Обсуждать "чистый код" на примере подходов, используемых в yii, бесполезно.
Да ничего я там от yii не использую. Это общий код. В сервисе не использую репозиторий чтобы не загромождать пример, данные получаются через AR, это не только в yii так..
 

fixxxer

К.О.
Партнер клуба
Да на таких примерах вообще нет смысла что-либо рассматривать.
actionUpdate, ну. Это же просто обертка вокруг SQL-запросов с валидацией, никакой бизнес-логики. Слишком примитивная задача, для ее решения вообще ООП не нужно, можно и банальной процедуркой нормально сделать.

А вот где "// что то делаем дополнительное" - вот это "дополнительное" может оказаться самым интересным. Я очень часто вижу, как в проектах с актив-рекордами там как раз оказываются куски бизнес-логики, утекшие в приложение (в твоем случае, правда, непонятно, зачем вообще этот блок, он дублирует обработку исключения ниже).
 
Последнее редактирование:

ivanov77

Новичок
actionUpdate, ну. Это же просто обертка вокруг SQL-запросов с валидацией, никакой бизнес-логики.

А вот где "// что то делаем дополнительное" - вот это "дополнительное" может оказаться самым интересным. Я очень часто вижу, как в проектах с актив-рекордами там как раз оказываются куски бизнес-логики,
actionUpdate - web контроллер, UI слой, какая там бизнес логика? :)
Там только логика работы с текущим пользователем, в основном - валидация формы, а бизнес уже в сервисе.

Про checkIfExistsById это ж я как раз и интересуюсь, непонятно почему в том видео они идут на такие телодвижения.
 

Adelf

Administrator
Команда форума
Это стандартная ошибка, когда вроде бы как выделяют Service layer, но его клиенты знаю слишком дофига.

PHP:
class ArticleService
{
    public function update($id, array $formData)
    {
        $article = ArticleARModel::findOne(($id))
        if ( !$article)
          throw new DomainException('Article not found');
        }

        $article->setAttributes($formData->getAttributes());
        //additional actions
        //...
        $article->save();
    }
}
PHP:
class ArticleController extends \yii\web\Controller
{
    /**
     * @var ArticleService
     */
    private $service;

    public function actionUpdate($id)
    {
        try {        
        $form = new ArticleForm();
        $form->load(Yii::$app->request->post());
        if ($form->validate()) {
            $this->service->update($id, $form);
            $this->redirect('article', [$article->id ]);
        }
        } catch (DomainException $e) {
            throw new HttpNotFoundException('Article not found');
        }
    }

}
 

ivanov77

Новичок
Это стандартная ошибка, когда вроде бы как выделяют Service layer, но его клиенты знаю слишком дофига.
Я тоже думал сначала с формой поработать, а передавать чисто id-шку, видел что так в основном делают.
Но в таком коде есть нелогичность:
Форма то всегда будет валидироваться (возможно не по простому, а со многими запросами к БД, на уникальность и т.д.) даже когда это бессмысленно изначально т.к. такой модели не существует. На этот момент что, забить?
 

fixxxer

К.О.
Партнер клуба
Я бы даже уточнил, не надо путать (и тем более смешивать в одну кучу) валидацию формы с внутренней валидацией модели и со всякими unique constraints в базе. Это три большие разницы.

Нарушение DRY тут кажущееся и на самом деле его нет - это разные слои.

И, да, проверка на уникальность в базе в большинстве Rails-подобных реализаций еще и бессмысленна, поскольку делается не в общей транзакции с последующей записью и без locking read. Исключение все равно может вылететь, и его все равно надо корректно обрабатывать.
 
Последнее редактирование:

Adelf

Administrator
Команда форума
И, да, проверка на уникальность в базе в большинстве Rails-подобных реализаций еще и бессмысленна, поскольку делается не в общей транзакции с последующей записью и без locking read.
Во. Хороший аргумент :)
 

ivanov77

Новичок
Я бы даже уточнил, не надо путать (и тем более смешивать в одну кучу) валидацию формы с внутренней валидацией модели и со всякими unique constraints в базе. Это три большие разницы.
А как проводить внутреннюю валидацию модели? В самой же модели, не в сервисе?
Если это например AR такая которая не светит своими св-вами, в сеттере можно проверку сделать и кинуть исключение.
Хотя, проблема что сразу по первому несоответствию кинет, остальные проигнорирует. Значит так отпадает.
Или если это модель как в Yii, без сеттеров для св-в, то в ней метод предусмотреть updateData(вызываемый из сервиса) и в нем уже валидацию и тоже бросать исключение или исключения...
Нет у вас примеров кода как такие исключения, особенно по нескольким ошибкам сразу, кидаются и получается уже в контроллере ловятся и ошибки валидации добавляются в форму?
 

AnrDaemon

Продвинутый новичок
Всё зависит от того, как твоя можель создаётся.
Если последовательным редактированием свойств - придётся валидатор вручную гонять.
Если пачкой из реквеста - сделай фабрику, которая и будет гонять валидатор.
Впрочем, первый случай тривиально сводится ко второму.
 

fixxxer

К.О.
Партнер клуба
А как проводить внутреннюю валидацию модели?
Элементарно:
PHP:
class User
{

    public function changeEmail($email)
    {
         Assert::isEmail($email); // throws exception
         // ...
    }

}
ошибки валидации добавляются в форму
Ошибки валидации на уровне интерфейса - это специфика приложения. В веб-приложении, в бэкенде для мобильного приложения и в API для сторонних приложений может быть совершенно разная интерфейсная логика со своей спецификой валидации, и это забота приложения - вызывать методы моделей с корректными аргументами. Если неверно отформатированные данные дошли до домена - это уже ненормальная ситуация, тут достаточно просто кинуть исключение.

Плюс, надо понимать, что бывают интерфейсные нюансы, для которых правила валидации совсем другие. Банальный пример - ввод пароля при регистрации. Можно сделать повторный ввод пароля, можно сделать инпут с "глазиком" - это все детали интерфейса, не имеющие отношения к модели. Ну или вот такой пример: модель ожидает номер телефона в международном формате без плюсика в начале и без разделителей, а интерфейс может позволять ввод в произвольном виде, приводить к "каноничному" виду, обрабатывать ситуации типа "если пользователь из России и ввел восьмерку в начале - это то же, что +7"...
 

Вурдалак

Продвинутый новичок
@Adelf @fixxxer
Не хочется обсуждать вопрос валидации модели в рамках этой темы, но всё же.
Иногда хочется исключение, когда входные данные явно не имеют смысла (-42 Кельвина).
Но иногда хочется использовать события (исключение — это тоже своего рода событие, но прерывающее текущий поток выполнения программы) для валидации.

Например, вы подсчитываете количество неудачных попыток ввода пароля: $member->authenticate($password, ...).
Если кидать исключение при неверном пароле, то сущность не сохранится без соответствующих костылей.
Поэтому проще сделать так, чтобы клиент получал список событий, которые произошли (PasswordIsNotValid).

Если событие-ошибка идеально вписывается в предметную область, то тут можно особо и не думать.
Но вот события типа EmailIsNotEmail — это, конечно, полный треш и тут нужны исключения.
Но иногда выбор неочевиден.
 

Вурдалак

Продвинутый новичок
Поэтому я бы условно разделил валидацию самой модели на две:
1. Ограничения, в рамках которой модель имеет смысл. Если тут фейл, то это баг.
2. Бизнес-ошибки, которые желательно выражать в виде событий.

Как следствие, нужно уметь как-то возвращать список событий клиенту.
 

ivanov77

Новичок
Если неверно отформатированные данные дошли до домена - это уже ненормальная ситуация, тут достаточно просто кинуть исключение.
Вот я так тоже думал, почему это некорректные данные должны с формы поступать в сервис и через него в модель.
Но как в общем случае проверить что данные корректные, email не занят в т.ч., без базы?

Про нахлесты параллельных проверок и сохранений, ситуация очень редкая, по умолчанию ее специально обрабатывать не вижу смысла, но даже если и произойдет, то будет db exception -> transaction rollback -> сообщение об ошибке и попробуйте снова...

Все те разные валидации для разных приложений не проблема, форму же никто не заставляет делать одну, главное что в модель они поставляют правильные данные.

Не надо в валидации лезть в базу
Если вот так разделять, что валидация формы не лезет в базу, то это уневозможит очень нужный кейс - показать пользователю как можно много ошибок для исправления.
Есть например форма
Код:
email
phone
На них в форме только форматы прописаны.
Начало проверять, email прошел, phone - нет и ваш if ($form->validate()) уже не пустит внутрь на дальнейшие проверки, покажет только что phone ошибочен, хотя этот email тоже может быть некорректным ( занят).
 

Adelf

Administrator
Команда форума
PHP:
class User
{
    public function changeEmail(Email $email)
    {
         // ...
    }
}
Мне так больше нравится :)
а про события... не знаю. Как то привычнее, что пользователь ожидает два события - все ок, все плохо и вот текст причины. Но это надо обдумать... Anyway, лучше просто Exception со списком причин неудачи...

Все эти email&phone по большому счету домена не касаются. это задача именно Application(Service) layer - обеспечить уникальность их. Сущность вполне может жить с каким угодно правильным email и phone. Эти проверки не доменные. и там(Application layer) можно сделать все как надо.проверить все и вернуть список. @ivanov77, просто не путай проверку формы и всяких уникальностей. Проверка формы должна проверить, что юзер не дебил и все ввел верно.а уникальность - это следующий этап. Представь,что ArticleService у тебя лежит вообще на другой машине. а Controller просто обращается к нему по API. И не знает про базу данных ничего. И все станет проще для восприятия.
 
Сверху