Небольшое код ревью, тесты и рефакторинг в Laravel. Плохой/хороший коД

[@tinemak]

4:20 Я не знаком с принципами написания тестов в Laravel. Но всё же хочу отметить, что несколько логически отдельных тестов следует оформлять в отдельных блоках (методах). В вашем случае, я вижу 3 теста: "пустой запрос", "запрос с пустым полем `cities`", "правильный запрос".

[@CutCodeRu]

Правильно замечание, здесь тесты нуждаются в отдельном рефакторинге! В данном случае набросанны для ролика, думаю основная суть была донесена

[@aplokhy]

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

[@alexredcross]

Приятно смотреть такие рубрики, а ещё когда тесты адекватно работают, мало кто пишет код и ещё показывает его покрытым тестами, спасибо Данил :)

[@wotanweb]

Идеальный формат! Хотелось бы больше таких видео

[@devvv747]

Поддерживаю!

[@progtime2000]

Все понятно! Интересно было увидеть видео по тестирование или серию видео включающие unit, интеграционные и браузенрные тесты

[@CutCodeRu]

Поставлю в ближайшие планы

[@Garkolym]

Очень хороший формат, пожалуйста, продолжай.👍

[@dr_freeman]

Огооонь! Рубрика - огонь! Я бы хоть каждый день смотрел! ❤

[@CutCodeRu]

спасибо за поддержку. буду стараться выпускать чаще!

[@Naikshy]

Думал тоже за тесты сказать но вижу уже всё прояснили в комментариях, отлично) Очень удачная длинна ролика, не утомляет смотреть, спасибо

[@АленаЕршова-ъ5ю]

Спасибо. интересно! Побольше бы видео с код ревью. А еще лучше на что обращаться внимание при код ревью)

[@rpy6ocTb]

Рубрика отличная, жду продолжения

[@TsA1ex]

6:22 Нет возвращаемого типа в rules(), бесполезные php docs можно смело удалять. 6:33 В контроллере можно использовать $request->validated() вместо магического геттера, полные FQCN можно и нужно сокращать (линия справа в редакторе не просто так проведена) 6:39 Что будет, если во входящих данных не будет нужного ключа в массиве? Рекомендую использовать Arr::get() для получения данных в массиве. Я так понимаю при любой ошибке импорт свалится. UpdateOrCreate может всё же лучше будет, так если объем $data будет большой, то будет провал по памяти, а так шанс что хоть часть данных зайдёт. Хотя тут уже от задачи зависит. Предпочтительно при большом объеме импорта работать с чанками 7:32 Куда пропали поля type_id и region_id после рефакторинга? Они не нужны в БД? Удачного кодинга

[@CutCodeRu]

все верно а в плане количества данных в этом случае их не может быть много поэтому upsert подходит идеально

[@serhiisecurity]

Все верно, код не стал более читаем, после вас еще нужно рефакторить :)

[@nikitasomusev799]

Отлично, спасибо за контент. Хотелось бы увидеть разные виды тестирований. На скок я понимаю, тут feature tests

[@CutCodeRu]

Думаю о тестах будет отдельный ролик

[@zosyanax]

Топовая рубрика, однозначно продолжать!

[@КоролеваЗамков]

Спасибо. Рубрика отличная. Продолжайте пожалуйста))

[@CutCodeRu]

Завтра еще один ролик этой рубрики выйдет)

[@АлександрНаумов-ц2я]

Классно, побольше бы таких видосов по рефакторингу

[@CutCodeRu]

буду периодически выпускать!

[@rosamarsky]

1. TDD - не паттерн, а подход к разработке. 2. Сам то в FormRequest'ах возвращаемые типы не указываешь) 3. Создавать коллекцию, чтобы вызвать map(), а потом снова перевести в массив... нууу.. array_map() для слабаков?:) 4. public function handle($cities) { ... } - тайпхинт ```array $cities``` 5. function (array $city) { ... } - да-да, в замыканиях тоже указываем тип. 6. Возвращаемый тип в контроллерах: у тебя указаны юзейджи, необязательно писать полный namespace.

[@CutCodeRu]

1. Оговорился (умею, практикую) 2. Эх 3. Коллекции one love)) 4. Эх 5. Ну ты понял 6. Мелочи Все четко, спасибо!

[@KibokoKwembamba]

Шикарная рубрика!!! Оставляем всё как есть

[@ДарьяМилова-щ6ц]

Отличный формат!!!! Для новичка просто находка !!!!

[@socialreklamaru]

Формат отличный, продолжай! Напиши тест, где в cities передаётся строка “test”, кое-что нужно дописать)

[@levdau]

Рубрика что надо! 👏

[@CutCodeRu]

🙂

[@nt2548]

Спасибо большое. Продолжайте. Плюсую

[@CutCodeRu]

👌

[@НиколайШи-с9о]

где тайпхин переменной у handle? замыкание в map можно указать статичным, оно не зависит от текущего окружения, можно указать его тайпхинты

[@CutCodeRu]

Спасибо за внимательность! И грамотное дополнение насчет static

[@egorow4innikow565]

Однозначно стоит продолжать. Круто

[@dilanhasck1447]

Отличная рубрика, увликательная 🎉

[@Seana_Black]

очень полезная рубрика!!!

[@CutCodeRu]

спасибо за поддержку!

[@ivan.silicin]

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

[@CutCodeRu]

Иделально редко бывает но постепенно стремится к совершенству) а так где-то что-то упустил в процессе, где-то не так сказал! На звание гения не претендуем

[@ym-nik-1343]

Та норм, если все идеально будет то где ж учиться на ошибках. Сначала увидел как плохо, потом посмотрел как сделать хорошо, а потом в комментах узнал, как можно еще лучше. Лично я всегда иду читать комментарии и думаю не я такой один.

[@bizargz6152]

Спасибо. Довольно интересно

[@ym-nik-1343]

Отличная рубрика, прям очень зашло Еще интересует организация кода. Понятно, что контроллер должен быть тонким, но куда и в какой структуре сложить бизнес-логику, которая отвечает за получение данных, сохранение в БД общение с другими сервисами и тд. У тебя заметил Action для этого. Что еще можешь порекомендовать, может видео статью или книгу на эту тему. Заранее спасибо

[@CutCodeRu]

Я сделаю видео об этом

[@velman2349]

супер! сделай подробный ролик о написании тестов.

[@CutCodeRu]

👍

[@x0tta6bl48]

Отличное видео, только один момент не понял. Из контроллера возвращаем + или -, а что вернётся если запрос не пойдет вадидацию?

[@atoomotr]

Да, это интересно. НО, если будете писать код вместо просто показа, то будет понятнее уследить для новичков. Спасибо за материал!

[@CutCodeRu]

Будет время будем писать! Стараюсь по максимому! Спасибо за просмотр и отзыв

[@konstantinchernishov1930]

Рефакторинг огромного файла с роутами в студию пожалуйста=)

[@CutCodeRu]

Слушаюсь и повинуюсь)

[@MsMavrin]

Хороший формат

[@nikoskullez]

Идея супер, можно будет в будущем присылать свой код?

[@CutCodeRu]

Почему бы и нет)

[@lexxkrt]

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

[@pilyugin]

Не знал, про prepare в реквесте, пойду читать😁

[@pilyugin]

Понял, в 5.7 этого метода нет... когда уже тимлид созреет на апдейт)))

[@CutCodeRu]

@@pilyugin ого как вы далеко застряли в прошлом)

[@mnogokotin]

спасибо )

[@ЮрийПлохов-к2в]

Классный формат. Продолжай пожалуйста. Только зачем возвращаемое значение в методах контроллера указывать?

[@CutCodeRu]

чтобы статический анализатор не ругался)) и надо привыкать везде проставлять типы) старые плохие привычки не во благо

[@dontsmoking2128]

префакторю ваш рефакторинг)) // в топку $this->assertModelExists(); $this->assertDatabaseHas(City::class, ['name' => 'test']); такой вопросик, а что быдете делать если у вас роут (ну вдруг) сменится)) route('name')

[@CutCodeRu]

брошу разработку и уйду работать поваром) Ведь к такому повороту меня жизнь точно не готовила

[@dontsmoking2128]

@@CutCodeRu бизнесу пофик к чем нас готовили

[@CutCodeRu]

@@dontsmoking2128 суровый бизнес)

[@extrememod2734]

Зачем возвращать false? Почему не использовать Error handler? Да и true думаю тоже не стоит, но могу ошибаться Кстати почему slug не добавить в prepareForValidation? И как писалось ранее не передать валидированные данные? А так топ!

[@CutCodeRu]

Нет ничего плохого в return bool, на данном этапе ошибки не отлавливали... Slug приходит от црм в готовом виде, манипуляции с ним не требуются! А так это пример реализации, ваша может отличаться

[@erjan816]

Классная рубрика! Но мне кажется год уж совсем плохо был написан и очевидно что нужно было рефакторить

[@ТимофейИванов-м1ю]

Аффтор жжоот! Пеши исчо! )))

[@CutCodeRu]

спасибо за поддержку!

[@oWeRQ666]

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

[@mrfriz]

А я вот похейтить хочу. У меня нет опыта в команде и рефакторинге, НО! 1. Разве нет читаемые работало раньше? 2. Разве не притянуты за уши такие тесты? Нужно ведь глазами увидеть строку в БД, чтобы убедиться, что заполнены все поля 3. Почему менее нежно проверять вручную? 4. Почему не обработано исключение на json_decode? Я хочу оказаться неправым и понять почему. Я сам часто не чувствую как было бы написать правильнее, красивее и читаемее. Но в этой ситуации, ИМХО, читаемость упала. Рубрика огонь, продолжай! Видеть чужие комментарии и варианты написания кода - это то, чего я давно хотел, но негде было посмотреть. Или плохо искал.

[@CutCodeRu]

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

[@mrfriz]

@@CutCodeRu ой, автокомплитом писал с телевизора) 1. В исходном варианте понять что делает код было проще. Что стало лучше после рефакторинга? 2. Тесты могут быть пройдены даже в случае, если одно необязательное поле не заполнилось. Например, в коде до рефакторинга допустили бы ошибку в 36 строке: 'long' => $city['lng'], - попытка получить long вместо lng, из-за которой во всей выгрузке свойство long было бы null. Я правильно понимаю, что по-хорошему, все эти ситуации нужно покрыть тестами? Это целесообразнее ручного прогона через Postman, например? 3. Почему менее надёжно проверять вручную, чем тестами? 4. Вопрос снят. Я всегда думал, что json_decode возвращает исключение, если передан не json. Попутал с JS и всегда оборачивал его в try-catch

[@CutCodeRu]

@@mrfriz 1) в начальном примере низкоуровневый код который будет иметь проблемы со временем при том же написании тестов которые необходимы либо расширении или переносе логики в другое место, код не гибкий и не поддерживаемый что мы и пытались исправить, читаемость может но это только на раннем этапе 2) Тесты я набросал для ролика на основной функционал но само собой покрыть нужно больше кейсов и разнести все на отдельные методы, тесты нуждаются в дополнениях 3) Глупо каждый раз заглядывать в базу и проверять каждый кейс при любом изменении, гораздо проще проверять тестами и быть уверенным в поведении, плюс как насчет деплоя в продакшен? Предварительные тесты проверят все еще раз и в случае провала релиз не уйдет в бой и еще миллион причин для необходимости тестов, эта тема даже не должна подниматься) 4) Да и в целом здесь данные приходят от црм которые всегда стандартизированны и в целом даже в валидации не сильно нуждались

[@CutCodeRu]

@@mrfriz я смог пережить ваш хейт?)

[@mrfriz]

@@CutCodeRu да) Буду заглядывать на канал в ожидании новых видео о написании тестов на боевом проекте. Тончайший намёк) Реально не хватает в рунете видосов с боевых проектов. Везде абстрактные примеры и демонстрация функционала библиотек для тестирования.

[@pavelmgn]

Спасибо. Ко всему что писали в замечаниях, меня смущает вообще virtyal_name, можно ведь slug формировать из name? Не будет ли здесь дублирования slugов - это проверка в модели? Можно отправить сотни записей с одним id? Может distinct добавить? lat, lon только numeric? А если я введу 123124123.12, прокатит? Лучше regexp =) @throws Ну и понятно что это в общих чертах все и я придираюсь =)

[@CutCodeRu]

virtyal_name это поле которые приходит от црм, мы его не контроллируем а в рамках этого проекта уже slug

[@Heyb111]

Поясни за экшин класс, это просто рандомный кастомный класс куда ты вынес логику или же новая фича ларавел?

[@CutCodeRu]

к ларавел отношение не имеет, просто концепция разделения на класс которая уже привычна многим

[@glebsemenov6782]

Привет! Я php джун. Собираю закрытый чатик для начинающих php разработчиков. Будем нетворкаться, учить новые штуки вместе и мотивировать друг друга. Пиши свой тг в комменты.

[@CutCodeRu]

привет. вступай в наш открытый чатик - t.me/laravel_chat

[@maxulax]

не стоило в валидаторе декодировать. Валидатор валидирует

[@ym-nik-1343]

Это поведение заложено в ларку, я вам больше скажу: не только FormRequest имеет не одну ответственность, такая уж магия лараваль :) Если данные не декодировать то что тогда валидировать в FormRequest , наличие длинной строки похожей на json? Или предлагаете в контроллере декодировать а потом валидацию запускать? Или мидлвар под это дело создать? :) Реально интересно как бы вы поступили

[@CutCodeRu]

Конечно же вы правы но такое решение было приятным компромиссом) пожалуй оставлю

[@ВиталийИнчин]

Ору с тестов

[@CutCodeRu]

🤔