Небольшое код ревью, тесты и рефакторинг в Laravel. Плохой/хороший коД
HTML-код
- Опубликовано: 3 авг 2024
- Сегодня будем смотреть код реального проекта, где я работаю в команде с другими разработчиками. Сделаем код ревью и рефакторинг. Буду делиться с Вами процессом работы
#рефакторинг#laravel#cutcode
---------------------------------------------------------------------------------
❗️❗️❗️Присоединяйся к нашему комьюнити в телеграм - там и советом помогут и много интересного - t.me/laravel_chat
🤖🤖🤖Мой помощник Тэйлор готов выдать тебе подарок. Забирать тут - cutcode.ru/chat-bot
---------------------------------------------------------------------------------
⏰ Таймкоды:
00:00 Введение
00:43 Обзор проекта
04:10 Создание тестов
05:53 Рефакторинг
Всех приветствую на канале Cutcode! Сегодня стартует новая рубрика, которую я назвал хороший плохой код. Будем смотреть на код, который имеет вопросы, серьезные вопросы, либо с небольшим запашком. И рассматривать детально проблемы, а потом буду демонстрировать код после рефакторинга. В общем и плохой и хороший код нас ждет сегодня. Вы обязательно напишите имеет ли это рубрика право на существование, либо предложите свое решение, если увидеть еще проблемы. Все это приветствуется, обязательно с вами обсудим. Но меньше слов друзья - погнали!
На обзоре у нас сегодня реальный проект, работаем в команде и в процессе рефакторинга и code review сразу делюсь с вами. Думаю это заслуживает хотя бы лайка.
Итак у нас класс контроллер, который отвечает за импорт данных из crm. Данные у нас приходят в json формате каждую ночь. Да возможно в целом подход не из лучших, но мы иногда привязаны к обстоятельствам и особенностям ТЗ. Импорт реализовал один из разработчиков моей команды и скажем так у нас живое code review по проблемам которые здесь есть. Сразу скажу что код рабочий импорт работает без ошибок. Но все-таки что же с ним не так? Во-первых что самое странное, это то, что код писался без тестов. Хотя на мой взгляд и я бы пошел именно таким путем, я бы использовал TDD паттерн и начал именно с тестов. Почему? Ну смотрите разработчик явно действовал следующим образом: писал код, а проверял пуля тестовый запрос либо прямо из crm, либо эмулировал через скажем postman в итоге каждый раз при любом изменении отправлял запрос и ждал ошибку, а в случае если ее нет, то смотрел все ли хорошо в базе, все ли там создалось и именно так как нужно. Лично я слишком ленив для такого подхода и определенно сразу бы написал тесты и далее контролировал бы поведение за счет тестов. Видел бы что запрос прошел валидацию, что все записи создались и они имеют правильные значения. Здесь же помимо того что каждый раз нужно самостоятельно все смотреть, но даже при таком подходе ошибиться и не углядеть что-то крайне легко. Это во-первых и это крайне критично. Во-вторых мне не нравится что здесь нет валидации данных и что мы здесь сразу делаем декодирование, хотя можно было бы перенести этот процесс в отдельный класс как раз валидации. В-третьих у нас идет метод апдейт либо create по полю ID. ID у нас явно уникальное поле. Дублей здесь быть не может и мы получается что на каждый город сразу отправляем к базе два запроса на поиск записи и на обновление либо добавление.
---------------------------------------------------------------------------------
📹 делитесь этим видео с друзьями:
• Небольшое код ревью, т...
zen.yandex.ru/video/watch/622...
🔔 подпишитесь на RUclips-канал: ruclips.net/user/CutCodeRu?s...
📼 Курс по Laravel с нуля:
• Курс по Laravel 8 обуч...
Небольшое код ревью, тесты и рефакторинг в Laravel. Плохой/хороший коД
---------------------------------------------------------------------------------
🔗 наш сайт: cutcode.ru/
📷 наш instagram: / cutcoderu
📱 Наш telegram-канал: t.me/laravel_cutcode
Приятно смотреть такие рубрики, а ещё когда тесты адекватно работают, мало кто пишет код и ещё показывает его покрытым тестами, спасибо Данил :)
4:20 Я не знаком с принципами написания тестов в Laravel. Но всё же хочу отметить, что несколько логически отдельных тестов следует оформлять в отдельных блоках (методах). В вашем случае, я вижу 3 теста: "пустой запрос", "запрос с пустым полем `cities`", "правильный запрос".
Правильно замечание, здесь тесты нуждаются в отдельном рефакторинге! В данном случае набросанны для ролика, думаю основная суть была донесена
Отличная рубрика, в сети не хватает примеров именно с реальными проектами, а так твой канал выгодно отличается от других, продолжай)
Очень хороший формат, пожалуйста, продолжай.👍
Идеальный формат!
Хотелось бы больше таких видео
Поддерживаю!
Спасибо. интересно! Побольше бы видео с код ревью. А еще лучше на что обращаться внимание при код ревью)
Все понятно! Интересно было увидеть видео по тестирование или серию видео включающие unit, интеграционные и браузенрные тесты
Поставлю в ближайшие планы
1. TDD - не паттерн, а подход к разработке.
2. Сам то в FormRequest'ах возвращаемые типы не указываешь)
3. Создавать коллекцию, чтобы вызвать map(), а потом снова перевести в массив... нууу.. array_map() для слабаков?:)
4. public function handle($cities) { ... } - тайпхинт ```array $cities```
5. function (array $city) { ... } - да-да, в замыканиях тоже указываем тип.
6. Возвращаемый тип в контроллерах: у тебя указаны юзейджи, необязательно писать полный namespace.
1. Оговорился (умею, практикую)
2. Эх
3. Коллекции one love))
4. Эх
5. Ну ты понял
6. Мелочи
Все четко, спасибо!
6:22 Нет возвращаемого типа в rules(), бесполезные php docs можно смело удалять. 6:33 В контроллере можно использовать $request->validated() вместо магического геттера, полные FQCN можно и нужно сокращать (линия справа в редакторе не просто так проведена)
6:39 Что будет, если во входящих данных не будет нужного ключа в массиве? Рекомендую использовать Arr::get() для получения данных в массиве. Я так понимаю при любой ошибке импорт свалится. UpdateOrCreate может всё же лучше будет, так если объем $data будет большой, то будет провал по памяти, а так шанс что хоть часть данных зайдёт. Хотя тут уже от задачи зависит. Предпочтительно при большом объеме импорта работать с чанками
7:32 Куда пропали поля type_id и region_id после рефакторинга? Они не нужны в БД?
Удачного кодинга
все верно а в плане количества данных в этом случае их не может быть много поэтому upsert подходит идеально
Все верно, код не стал более читаем, после вас еще нужно рефакторить :)
Думал тоже за тесты сказать но вижу уже всё прояснили в комментариях, отлично)
Очень удачная длинна ролика, не утомляет смотреть, спасибо
Рубрика отличная, жду продолжения
Шикарная рубрика!!! Оставляем всё как есть
Топовая рубрика, однозначно продолжать!
очень полезная рубрика!!!
спасибо за поддержку!
Рубрика что надо! 👏
🙂
Отлично, спасибо за контент.
Хотелось бы увидеть разные виды тестирований. На скок я понимаю, тут feature tests
Думаю о тестах будет отдельный ролик
Спасибо. Рубрика отличная. Продолжайте пожалуйста))
Завтра еще один ролик этой рубрики выйдет)
Спасибо большое. Продолжайте. Плюсую
👌
Однозначно стоит продолжать. Круто
Спасибо. Довольно интересно
Отличный формат!!!! Для новичка просто находка !!!!
Формат отличный, продолжай!
Напиши тест, где в cities передаётся строка “test”, кое-что нужно дописать)
Отличная рубрика, увликательная 🎉
Классно, побольше бы таких видосов по рефакторингу
буду периодически выпускать!
Рефакторинг огромного файла с роутами в студию пожалуйста=)
Слушаюсь и повинуюсь)
где тайпхин переменной у handle?
замыкание в map можно указать статичным, оно не зависит от текущего окружения, можно указать его тайпхинты
Спасибо за внимательность! И грамотное дополнение насчет static
спасибо )
Хороший формат
Да, это интересно. НО, если будете писать код вместо просто показа, то будет понятнее уследить для новичков.
Спасибо за материал!
Будет время будем писать! Стараюсь по максимому! Спасибо за просмотр и отзыв
супер! сделай подробный ролик о написании тестов.
👍
Тема правильная, очень нужны такие видео. Но почитав комментарии появилось чувство несовершенства. В видео по код ревью все должно быть идеально без оговорок.
Лайк.
Иделально редко бывает но постепенно стремится к совершенству) а так где-то что-то упустил в процессе, где-то не так сказал! На звание гения не претендуем
Та норм, если все идеально будет то где ж учиться на ошибках.
Сначала увидел как плохо, потом посмотрел как сделать хорошо, а потом в комментах узнал, как можно еще лучше.
Лично я всегда иду читать комментарии и думаю не я такой один.
Отличная рубрика, прям очень зашло
Еще интересует организация кода. Понятно, что контроллер должен быть тонким, но куда и в какой структуре сложить бизнес-логику, которая отвечает за получение данных, сохранение в БД общение с другими сервисами и тд.
У тебя заметил Action для этого. Что еще можешь порекомендовать, может видео статью или книгу на эту тему. Заранее спасибо
Я сделаю видео об этом
Классная рубрика! Но мне кажется год уж совсем плохо был написан и очевидно что нужно было рефакторить
Не знал, про prepare в реквесте, пойду читать😁
Понял, в 5.7 этого метода нет... когда уже тимлид созреет на апдейт)))
@@pilyugin ого как вы далеко застряли в прошлом)
Идея супер, можно будет в будущем присылать свой код?
Почему бы и нет)
Отличное видео, только один момент не понял. Из контроллера возвращаем + или -, а что вернётся если запрос не пойдет вадидацию?
префакторю ваш рефакторинг))
// в топку $this->assertModelExists();
$this->assertDatabaseHas(City::class, ['name' => 'test']);
такой вопросик, а что быдете делать если у вас роут (ну вдруг) сменится))
route('name')
брошу разработку и уйду работать поваром) Ведь к такому повороту меня жизнь точно не готовила
@@CutCodeRu бизнесу пофик к чем нас готовили
@@dontsmoking2128 суровый бизнес)
В map’e лучше было полностью описать массив, как было до, это будет являться самодокументацией, потом гораздо проще с таким кодом работать, да и в общем случае валидируются только необходимые данные, а помимо них может добавиться что-то еще.
Аффтор жжоот! Пеши исчо! )))
спасибо за поддержку!
Классный формат. Продолжай пожалуйста. Только зачем возвращаемое значение в методах контроллера указывать?
чтобы статический анализатор не ругался)) и надо привыкать везде проставлять типы) старые плохие привычки не во благо
Зачем возвращать false? Почему не использовать Error handler? Да и true думаю тоже не стоит, но могу ошибаться
Кстати почему slug не добавить в prepareForValidation? И как писалось ранее не передать валидированные данные?
А так топ!
Нет ничего плохого в return bool, на данном этапе ошибки не отлавливали...
Slug приходит от црм в готовом виде, манипуляции с ним не требуются! А так это пример реализации, ваша может отличаться
Спасибо. Ко всему что писали в замечаниях, меня смущает вообще virtyal_name, можно ведь slug формировать из name? Не будет ли здесь дублирования slugов - это проверка в модели? Можно отправить сотни записей с одним id? Может distinct добавить? lat, lon только numeric? А если я введу 123124123.12, прокатит? Лучше regexp =) @throws Ну и понятно что это в общих чертах все и я придираюсь =)
virtyal_name это поле которые приходит от црм, мы его не контроллируем а в рамках этого проекта уже slug
Поясни за экшин класс, это просто рандомный кастомный класс куда ты вынес логику или же новая фича ларавел?
к ларавел отношение не имеет, просто концепция разделения на класс которая уже привычна многим
А я вот похейтить хочу. У меня нет опыта в команде и рефакторинге, НО!
1. Разве нет читаемые работало раньше?
2. Разве не притянуты за уши такие тесты? Нужно ведь глазами увидеть строку в БД, чтобы убедиться, что заполнены все поля
3. Почему менее нежно проверять вручную?
4. Почему не обработано исключение на json_decode?
Я хочу оказаться неправым и понять почему. Я сам часто не чувствую как было бы написать правильнее, красивее и читаемее. Но в этой ситуации, ИМХО, читаемость упала. Рубрика огонь, продолжай! Видеть чужие комментарии и варианты написания кода - это то, чего я давно хотел, но негде было посмотреть. Или плохо искал.
Я некоторые пункты не понял, перефразируйте пожалуйста чтобы я мог ответить
@@CutCodeRu ой, автокомплитом писал с телевизора)
1. В исходном варианте понять что делает код было проще. Что стало лучше после рефакторинга?
2. Тесты могут быть пройдены даже в случае, если одно необязательное поле не заполнилось. Например, в коде до рефакторинга допустили бы ошибку в 36 строке: 'long' => $city['lng'], - попытка получить long вместо lng, из-за которой во всей выгрузке свойство long было бы null. Я правильно понимаю, что по-хорошему, все эти ситуации нужно покрыть тестами? Это целесообразнее ручного прогона через Postman, например?
3. Почему менее надёжно проверять вручную, чем тестами?
4. Вопрос снят. Я всегда думал, что json_decode возвращает исключение, если передан не json. Попутал с JS и всегда оборачивал его в try-catch
@@mrfriz
1) в начальном примере низкоуровневый код который будет иметь проблемы со временем при том же написании тестов которые необходимы либо расширении или переносе логики в другое место, код не гибкий и не поддерживаемый что мы и пытались исправить, читаемость может но это только на раннем этапе
2) Тесты я набросал для ролика на основной функционал но само собой покрыть нужно больше кейсов и разнести все на отдельные методы, тесты нуждаются в дополнениях
3) Глупо каждый раз заглядывать в базу и проверять каждый кейс при любом изменении, гораздо проще проверять тестами и быть уверенным в поведении, плюс как насчет деплоя в продакшен? Предварительные тесты проверят все еще раз и в случае провала релиз не уйдет в бой и еще миллион причин для необходимости тестов, эта тема даже не должна подниматься)
4) Да и в целом здесь данные приходят от црм которые всегда стандартизированны и в целом даже в валидации не сильно нуждались
@@mrfriz я смог пережить ваш хейт?)
@@CutCodeRu да) Буду заглядывать на канал в ожидании новых видео о написании тестов на боевом проекте. Тончайший намёк)
Реально не хватает в рунете видосов с боевых проектов. Везде абстрактные примеры и демонстрация функционала библиотек для тестирования.
Ору с тестов
🤔
Привет! Я php джун. Собираю закрытый чатик для начинающих php разработчиков. Будем нетворкаться, учить новые штуки вместе и мотивировать друг друга. Пиши свой тг в комменты.
привет. вступай в наш открытый чатик - t.me/laravel_chat
не стоило в валидаторе декодировать. Валидатор валидирует
Это поведение заложено в ларку, я вам больше скажу: не только FormRequest имеет не одну ответственность, такая уж магия лараваль :)
Если данные не декодировать то что тогда валидировать в FormRequest , наличие длинной строки похожей на json?
Или предлагаете в контроллере декодировать а потом валидацию запускать?
Или мидлвар под это дело создать? :)
Реально интересно как бы вы поступили
Конечно же вы правы но такое решение было приятным компромиссом) пожалуй оставлю