Правильно ли выбрасывать исключения в бизнес логике?

Question

[dmitriy]

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

try{
   $order = $orderService->createOrderAndNotify($orderRequest);
   return $this->view("success",[
      "orderId" => $order->getId()
   ]);
}
catch(OrderValidationException $ex){
   $logger->warning("error valid order", $ex->getErrors());
   return $this->view("warning",[
       "errors" => $ex->getErrors()
   ]);
}
catch(OrderException $ex){
   $logger->error($ex->getMessage());
   return $this->view("error");
}

иерархия исключений Order\Exceptions

OrderException extends RuntimeException
OrderValidationException extends OrderException
OrderNotFound extends OrderException

вижу приемущества:
1. конкретный ответ метода, а не какой то специализированный DTO отдающий все на свете
2. обработка исключений в зависимости от ситуации и логики приложения
3. тестируемость и документированность (phpunit,docblock)
4. передача доп. данных в исключениях

Answer 1

[Алексей Николаев]

Я считаю это хорошей практикой. Исключительная ситуация - это та, при которой программа выполняется не так, как ожидалось. Но, позвольте, я ожидаю, что заказ пройдет валидацию. Я ожидаю, что заказ завершится успехом. Поэтому OrderValidationException, OrderException - исключительные ситуации. Исключительной ситуацией не является, например, отмена пользователем заказа - глупо выбрасывать исключение по клику на Cancel.

Исключения гораздо глубже, чем просто "исключительная ситуация". Они помогают развести обработку ошибок по разным уровням абстракции, а использование разных типов исключений позволяет обрабатывать конкретные ошибки там, где их нужно обработать (на нужном слое). Исключения очень помогают в локализации ошибки в дальнейшем, они позволяют избавиться от простыни If .. else, проверок на null, false и прочей ереси. Они, чаще всего, делают код чище.

Но пользоваться ими стоит аккуратно, все-таки стоит разделять исключительную ситуацию и вполне нормальную, штатную ветку исполнения. Так, если заказов нет - это НЕ исключение, тут можно вернуть null - допустимо (хотя некоторые и тут выбрасывают исключение, потому что очень соблазнительно привязать код исключения к HTTP кодам). А если у вас аплоад документов, и юзер пытается загрузить файл слишком большой, то это уже исключение.

Comments

[dmitriy]

Спасибо , как глоток воздуха

Answer 2

[FanatPHP]

Ответ на вопрос из заголовка простой: исключение выбрасывается тогда, когда код не в состоянии выполнить работу, для которой он предназначен. Это простое правило, общее для программирования в целом, и безнес логика в этом смысле ничем не отличается от любого другого кода.

Я бы разделил валидацию и создание заказа.
Для валидатора ошибки введенных данных - это не исключителная ситуация, а соверешнно штатная.
То есть можно проверить результат валидации простым условием, без всяких исключений.

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

Но вот код, который лежит внутри catch(OrderException $ex){ является избыточным. Логирование ошибки и вывод стандартного сообщения клиенту - это то, что должен делать централизованный обработчик ошибок, который в любом случае должен присутствовать в приложении. То есть здесь этот код явно лишний.

Comments

[Дмитрий Ким]

createOrderAndNotify тоже лучше разделить. Непонятно, должен ли создаваться заказ, если не получилось отправить уведомление. Или отправляется ли уведомление при несохраненном заказе.

[dmitriy]

а теперь смотрите у нас появляется rest контроллеры которые будут использовать туже логику но обрабатывать по другому

try{
   $order = $orderService->createOrderAndNotify($orderRequest);
   return new JsonResponse([
      "orderId"=>order ->getId()
      ], 200
   );
}
catch(OrderValidationException $ex){
   return new JsonResponse([
      "errors"=>$ex->getErrors()
      ], 400
   );
}
catch(OrderException $ex){
$logger->error($ex->getMessage());
   return new Response("application error", 500)
}

[dmitriy]

Дмитрий Ким, в том то и дело что это целостность операции если на каком то этапе произошел сбой то транзакция должна быть отменена, выброшено исключение

[FanatPHP]

dmitriy, я наверное соглашусь с первым кетчем, но вот второй тут 100% лишний.

Что будет делать твой API при появлении ошибки file not found, permission denied и прочих? Будет логировать ошибку и выдавать соответствующий респонс. Так зачем раздувать свой код на пустом месте ?

[dmitriy]

например есть РЕСТ запрос GET /api/tmpfile/{hash}
и есть метод бизнес логики получающий файл по хешу, так в контролере выходит все читабельно

try{
$file = $fileService->getFileByHash($hash);
}
catch(FileNotFoundException $ex){
return new Response("404 not found",404);
}

//дальше что то оделаем например отдаем контент файла

все случаи связанные с невозможносттю чтения файла будут оборачиваться в FileNotFoundException и отдаваться выше

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

[FanatPHP]

все случаи связанные с невозможносттю чтения файла будут оборачиваться в FileNotFoundException

Боже, как ты наивен.
Ладно, строчи свои кетчи дальше. Потом сам поймешь (если конечно тебе придётся самому пользоваться своей поделкой, а не сбагрить заказчику и забыть про неё) что ошибки бывают не только те, которые ты предусмотрел и красиво обвёл в рамочку.

[dmitriy]

очень рад буду увидеть пример такой исключительной ситуации, а не а не юношеского пустословия )

[FanatPHP]

Я привёл, два. Но ты их не понял, потому что у тебя мозги на других рельсах стоят. А самомнение не даёт им перевести стрелки. Расслабься, я тебя уже ни к чему не агитирую. С опытом все само придёт.

Answer 3

[Северное Сияние]

Нет, это плохая практика.

Исключительная ситуация на то и исключительная. Это ошибка запроса, нет коннекта к БД или отсутствие записи, которая, как мы уверены, должна существовать и тд. Это событие, после которого работа кода НЕВОЗМОЖНА.

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

Comments

[Северное Сияние]

tommy-vercetti, что ты там вякнул?

[Северное Сияние]

tommy-vercetti, ты аргументы приведешь относительно своей реплики или твой аргумент "сам дурак"?

[Северное Сияние]

tommy-vercetti, Вы кто такие, авторитеты, что бы ваши слова за истину принимать?

Мне не интересно рассказывать,ь почему исключения не надо использовать в бизнес-логике, это материал ООП первого курса института.

За фрика ты ответишь? Давай номер, позвоню, побазарим.

Answer 4

[Arman]

может интересно будет
https://github.com/adelf/acwa_book_ru/blob/master/...