@Anton_O_B
PHP программист

Может ли кто-то проревьюить ООП код на PHP (тестовая задача, Symfony)?

Есть решенная тестовая задача https://github.com/ab2021-dev/test-task, по которой отказ. Однако, технического фидбека не предоставили. Нужен сильный ООП-программист для review кода - кода не много (папка src всего 40 Кб), задача вроде бы простая. Но, видимо, не настолько чисто решена, как требуется.
  • Вопрос задан
  • 1182 просмотра
Пригласить эксперта
Ответы на вопрос 1
glaphire
@glaphire
PHP developer
Мне кажется что тестовое сделано хорошо, но есть вещи которые немного некомфортны для потенциального проверяющего:
1) Хорошо бы было все команды по поднятию проекта завернуть в Makefile или bash сценарий
2) EmployeeScheduleController Аннотация Route на уровне класса это похоже на оверхед здесь, потому что только два эндпоинта и пустой роут над getWorkSchedule сразу пугает)) И private методы лучше сместить все вниз или вынести в базовый контроллер.
3) Employee создание класса Time тоже больше похоже на оверхед, с одной стороны хорошо, что все ограничения инкапсулированы в одном классе, с другой стороны операции с DateTime как-то более интуитивны.
4) EmployeeRepository Метод loadEmployeesFromFile() это точно не зона ответственности доктриновского репозитория, это отдельный класс, обычно сервис. Репозиторий это слой чтения из хранилища, а тут процесс записи.
5) DayFactory, TimeFactory, TimeRangeFactory и их интерфейсы кажутся очень большим оверхедом, потому что на небольшую логику созданы три класса и три интерфейса, про которые надо помнить и проверять их содержимое для поддержания общей линии приложения.
6) Обычно в симфони стараются придерживаться прямолинейной структуры папок, внутри Service есть Builder, ExternalApi, Factory и Validation - это разные группы задач и точно не сервисы, стоило бы оставить их в неймспейсе App (папке src) или выделить папку Module/Scheduler и создавать эти папки там.
7) Calendar - у этого класса зона ответственности это быть апи клиентом, можно его переименовать в CalendarApiClient, чтобы однозначно понимать что он дергает внешнее апи, а не просто сущность, как-то связанная с другим апи.
8) Validation - это необязательно, но обычно стараются по-максимуму использовать компонент Validator и логику дополнительных проверок строить вокруг него
9) ScheduleDirector может я раньше не встречалась с такой группой классов, но Director звучит контринтуитивно, Service или Manager немного привычнее и предсказуемее.
Тут есть противоречние - свойство scheduleBuilder не сеттится сразу, хотя оно необходимо для фунционирования, при этом его надо сеттить обязательно через сторонний метод. В симфони можно сконфигурировать класс заранее в services.yaml, задать ему алиас и уже готовый инжектить в нужный класс. Немного странно видеть исключения BadRequestHttpException, NotFoundHttpException в классе, зоной ответственности которого не является работа с http напрямую)
10) EmployeeNonWorkScheduleControllerTest - зачем использовать в качестве названий переменных подчеркивания? Может это ошибка? Просто выглядит очень странно)
11) В комментариях упоминали добавить коллекцию постмана, это было бы желательно
Ответ написан
Ваш ответ на вопрос

Войдите, чтобы написать ответ

Войти через центр авторизации
Похожие вопросы