Чистый код. №2: Чистим функции. Теория и практика. По книге Роберта Мартина

Как сделать функции хорошо читаемыми? Функция должна быть короткой или очень короткой? А что значит: "короткая функция"? Чем плохи конструкции switch (match)? Почему так отвратительны аргументы-флаги? Как функция связана с уровнем абстракции и принципом единственной ответственности (Single Responsibility Principle)? Нуль-арные, унарные, бинарные, тернарные и полиарные функции - что это? Как правильно работать с исключениями (exceptions) и блоками try/catch? Сегодня обсудим всё это, опираясь, конечно же, на книгу Роберта Мартина "Чистый код".

А на десерт у нас - попытка рефакторинга реальной функции (метода контроллера из Laravel-приложения). Насколько удачной будет эта попытка? Давайте посмотрим.

Содержание ролика "Чистый код. №2: Чистим функции. Теория и практика. По книге Роберта Мартина":
00:00 Настоящий программист не боится функций в 300 строк.
02:00 Глава 3 "Функции" книги Мартина.
03:00 Пример трудно читаемой функции из книги.
04:00 Функции должны быть компактными!
04:30 Что значит: компактная функция?
05:50 Правило одной операции.
06:55 Один уровень абстракции на функцию.
09:10 Чтение кода сверху вниз: правило понижения.
10:13 Команды switch/match: не прибегнуть ли к полиморфизму?
15:10 Используйте содержательные имена.
15:35 Классификация функций в зависимости от количества принимаемых аругментов: нуль-арные, унарные, бинарные, тернарные, полиарные.
16:50 Что плохого в аргументах-флагах?
18:27 Объекты как аргументы.
19:50 Избавьтесь от побочных эффектов.
21:32 Используйте исключения вместо возвращения кодов ошибок.
23:28 Изолируйте блоки try/catch.
26:00 Возражение Дейкстре по поводу единственной точки входа/выхода.
27:35 Как научиться писать такие функции?
29:48 Практика: попытка рефакторинга метода контроллера из Laravel-приложения.

[viper_vlad]

Да, хорошо получилось.
Но если мы говорим про Ларавел, то есть пару моментов:

0. Ты не прав, мы можем кидать exception, сохранив интерфейс API.

1. Твой try $params = $request->validated() не отработает должным образом, т.к. метод validated возвращает список отвалидированных данных. А сама валидация происходит до вызова метода контроллера. И соответсвенно, клиенты нашего API в случае ошибки будут получать 422, вместо ожидаемого 200.
Если во всем проекте используется одинаковый формат вывода ошибок валидации данных (200 HTTP-код, плюс "resultCode" в body), то самый лучший вариант - переопределить глобально формат ответа в случае В Ларавел это делается довольно легко. И тогда нам не нужно будет в каждом контроллере отдельно json ответ писать с "resultCode" в случае ошибки валидации

2. Метод checkIfProductExists тоже не нужен. Проверку на уникальность следует вынести в CreateCatalogProductRequest. Для такой проверки в Laravel есть правило unique.
Т.е., что-то типа 'product_id' => 'required|unique:products' и 'sku' => 'required|unique:products'

3. persistProductInDB очень плохо получился. В том плане, что он возвращает массив с ответом. Почему функция, которая отвечает за сохранение данных в БД, взялась формировать ответ для респонса? persistProductInDB должна возвращать bool, т.е., должно быть return $newProduct->save(). А уже в createCatalogProductAction мы проверяем и формируем array для респонса.
А вообще, на мой взгляд, нужно очень осторожно относится к возврату результата в array. Если в рамках одного класса такой подоход еще более менее норм, но если ты будешь возвращать какой-то array из каких-то сторонних классов, то может произойти что угодно. Т.к., у массива нет четкой структуры и ты не можешь быть уверен, какие данные хранятся в массиве, который ты получил. А каждый раз проверять структура массива тоже не вариант.

[ВладимирМороз-вб]

Замечательное видео. Очень жду продолжения.

[ТимофейХайлайн]

На 6:20 автор сказал, что первый принцип солид - это что функция должна выполнять только одну четко оговоренную в имени функции операция, так вот в книги Чистая Архитектура от Мартина в 7 главе 79 странице написано, что это распространенное заблуждение, а а singe responsibility принцип говорит о том, что «модуль должен иметь одна и только одну причину для изменения»

[pep]

Спасибо за видео. Где же вы были раньше. Подача материала божественна. Вы учитель от бога.

[yashkevich]

Весьма полезное видео. Жду Тестов теперь на это дело)

[ihorrud]

Видео огонь! Чем дальше в лес, тем больше партизанов)))

[views-luph]

Большое спасибо за Ваш канал! Круто! Было бы здорово, чтобы вы делали трансляции, когда кодите реальный проект делаете + давали бы краткие комментарии. Настоящего кодинга на ютубе мало!! Еще Вы обещали когда-то раньше, если я правильно понял, показать как делаете рефакторинг!

[mspcvju]

Спасибо за материал! Было очень интересно послушать!

[pandalove]

С exception и json ответом, можно поступить так. Написать функцию обертку, которая будет в try catch вызывать основную функцию, и если все хорошо, то из функции получать сообщение и устанавливать resultCode, а иначе в catch получать ошибку и возвращать resultCode 1 и сообщение из ошибки. А уже после того, как с фронтом договоритесь, можно будет легко убрать эту обертку.

[vitaliy]

Видео понравилось, буду смотреть еще. Но явно перепутаны понятия уровня абстракции и функционал. Если этот момент понимать, то смотрится хорошо.

Лайк, коммент, колокольчик.

[romanchubich]

С именованием уже давно страдаю. В одном проекте у меня база с именами вида item_id. В коде я тоже сделал $item_id.
В другом исправил имена полей в бд на itemId, чтобы в коде было эстетичнее.
До сих пор маюсь между: ведь в бд item_id лучше выглядит. А если в бд и коде разные имена, так вообще с ума можно сойти.

[dmitryalinsky]

На счёт практического примера. Но сразу скажу, что посмотрел только исходную функцию и результат. По этому может вы и сами озвучили такой вариант в процессе.
Я недавно столкнулся не с рефакторингом, а с интеграцией одной платёжной системы и у них в API был тот же подход с ожидание статуса 200, но с разными кодами ответа и сообщениями. Немного подумав, я решил вынести все варианты ошибок в отдельный класс и унаследовать его от \Exception.
Вышло примерно так (названия специально изменил):

class AnyPaymentException extends \Exception
{
public function __construct(public array $error)
{
}

public const ERROR_1 = [
'code' => -1,
'message' => 'Ошибка 1',
];

public const ERROR_2 = [
'code' => -2,
'message' => 'Ошибка 2',
];

public const ERROR_3 = [
'code' => -3,
'message' => 'Ошибка 3',
];
}



В сервисе по обработке запроса я сделал примерно как и вы, раскидав все проверки по отдельным функциям. В случае проблемы, выкидывал AnyPaymentException, передавая соответствующую константу.
А в экшене контроллера я сделал try - catch блок, и в случае ошибки, выполнял роллбэк транзакции и возвращал $exception->error со статусом 200.

[shiryshev]

Было полезно, спасибо! Час пролетел незаметно.

[arta]

Вижу страдания на протяжении всего видео. Вам скорее проще было б переписать весь проект, чем рефачить этот код))) Но как всегда познавательно, спасибо!

[NuChamu]

Работал на маркетплейсе и бывали одинаковые SKU для разных товаров от разных поставщиков

[compolomus]

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

[ACLeo_UA]

В целом все хорошо и правильно, НО! С возвратом null плохо. Мартин вроде писал, НИКОГДА не возвращать null.

[Фанат-щь]

Эх научится бы работать с контроллерами и тому подобное, а вообще было бы интересно создать пример на php с расчётом разных оплат работников

[ЛеопольдКотов-кй]

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

[vitmih]

create_catElog_product() И эти люди учат правильному неймингу
))))