Can someone review OOP code in PHP (test task, Symfony)?

Question

symfony

There is a solved test task https://github.com/ab2021-dev/test-task , which was rejected. However, no technical feedback was provided. We need a strong OOP programmer to review the code - there is not much code (the src folder is only 40 Kb), the task seems to be simple. But, apparently, not so cleanly solved as required.

Answer 1

It seems to me that the test is done well, but there are things that are a little uncomfortable for a potential tester:
1) It would be nice to wrap all the commands for raising the project in a Makefile or bash script
2) EmployeeScheduleController The Route annotation at the class level is like an overhead here, because only two endpoints and an empty route over getWorkSchedule immediately scares)) And private methods are better to shift everything down or take it to the base controller.
3) Employee creation of the Time class is also more like an overhead, on the one hand it's good that all the restrictions are encapsulated in one class, on the other hand, operations with DateTime are somehow more intuitive.
4) EmployeeRepositoryThe loadEmployeesFromFile() method is definitely not the responsibility of the Doctrine repository, it is a separate class, usually a service. The repository is a layer of reading from the repository, and here is the writing process.
5) DayFactory, TimeFactory, TimeRangeFactory and their interfaces seem to be a very big overhead, because three classes and three interfaces are created for little logic, which you need to remember and check their contents to maintain the general line of the application.
6) Usually in symphony they try to adhere to a straightforward folder structure, inside the Service there is a Builder, ExternalApi, Factory and Validation - these are different groups of tasks and definitely not services, it would be worth leaving them in the App namespace (src folder) or select the Module / Scheduler folder and create those folders are there.
7) Calendar- for this class, the area of ​​​​responsibility is to be an api client, you can rename it to CalendarApiClient in order to clearly understand that it pulls an external api, and not just an entity somehow connected with another api.
8) Validation - this is optional, but usually they try to make the most of the Validator component and build the logic of additional checks around it
9) ScheduleDirector I may not have met such a group of classes before, but Director sounds counterintuitive, Service or Manager is a little more familiar and predictable.
Herethere are contradictory ones - the scheduleBuilder property is not set immediately, although it is necessary for functioning, and it must be set through a third-party method. In symphony, you can configure the class in advance in services.yaml, set an alias for it and ready to inject it into the desired class. It's a little strange to see BadRequestHttpException, NotFoundHttpException exceptions in a class whose area of ​​responsibility is not working with http directly)
10) EmployeeNonWorkScheduleControllerTest - why use underscores as variable names? Could this be a mistake? It just looks very strange)
11) In the comments it was mentioned to add a postman collection, this would be desirable