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

    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) В комментариях упоминали добавить коллекцию постмана, это было бы желательно
    Ответ написан
    9 комментариев