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

    @Anton_O_B Автор вопроса
    @tommy-vercetti
    Это все выглядит красиво пока не начнется маппинг данных из субд на эти объекты или persist/update записи. Я думаю не зря вы решили использовать json-файл в качестве persistence, чтобы избежать эти проблемы. И здесь даже вопрос не к VO.

    Тут не очень понял, какие могут быть проблемы с маппингом?
    Можно вместо файла использовать и базу. В данной задаче для маппинга потребуется только класс Value\Time для полей workStart и lunchStart в Entity\Employee и это решается добавлением custom mapping type при использовании Doctrine.
  • Может ли кто-то проревьюить ООП код на PHP (тестовая задача, Symfony)?

    @Anton_O_B Автор вопроса
    Daria Motorina
    ScheduleRequestSubscriber - валидация это не зона ответственности реагирования на ивент. Для валидации есть другие слои/способы и этапы - до попадания данных в экшен контроллера или уже внутри него, получается что при использовании ивентов эти процессы могут пойти параллельно, а не последовательно. В доке написано (ссылка) - "Overall, the purpose of the kernel.request event is either to create and return a Response directly, or to add information to the Request (e.g. setting the locale or setting some other information on the Request attributes).".


    У меня в первой версии кода валидация как раз была в контроллере. И я понимаю, что там, вроде как наиболее подходящее и логичное место. Но потом я сделал через event с той целью, что невалидные данные до контроллера даже не дойдут (т.е. как бы быстрый ответ). И вроде бы в документации такая возможность тоже рассматривается "to create and return a Response directly".
    Могли бы пояснить:
    Какие кейсы подходят для быстрого ответа в подписчиках на событие Route?
    Какой еще может быть слой/этап для валидации данных и "до попадания данных в экшен контроллера"?
    "получается что при использовании ивентов эти процессы могут пойти параллельно, а не последовательно" - вы имеете ввиду подзапросы в рамках одного запроса?
  • Может ли кто-то проревьюить ООП код на PHP (тестовая задача, Symfony)?

    @Anton_O_B Автор вопроса
    tommy-vercetti, Daria Motorina спасибо за подробные ответы.

    По поводу public/private методов: у Мартина обосновывается тем, что когда читается код, то вроде как удобнее сразу просмотреть private метод после public метода, чем скроллить вниз до того момента когда public методы закончатся. С точки зрения, клиентского кода, да, получается, что удобнее сначала public потом private. Вроде бы простой, но интересный момент)

    Для дат/времени сделал объекты-обертки, потому что хотелось все инкапсулировать, т.е. чтобы сервисы работали только с нужными объектами + более строгая типизация, как мне кажется. Фабрики для этих объектов сделал для разделения ответственности - т.е. сервисы, построения расписания делегируют всю работу с объектами фабрикам. Можно было бы использовать стандартные \DateTime, \DateTimeInterface, но тогда, на мой взгляд, это не было бы проектирование по модели - я имею ввиду, что сейчас объект-обертка отображает предметную область: Day - день в расписании, Time - время в расписании, TimeRange - интервал времени. Не понимаю, почему это оверинжиринг/оверхед?

    Про валидацию:
    8) я вроде бы как раз и использовал Validator компонент, точнее сервис-обертку над ним, который валидирует параметры запроса как массив и вызывается этот сервис в ScheduleRequestSubscriber

    "ScheduleRequestSubscriber - так делать нельзя." - поясните, пожалуйста, почему именно?

    "в какую компанию и на какую позицию вы собирались устроиться?" - UK стартап, PHP Symfony разработчик, наверное, подразумевалось, что senior, но точно не lead.
  • Может ли кто-то проревьюить ООП код на PHP (тестовая задача, Symfony)?

    @Anton_O_B Автор вопроса
    @glaphire
    4) тут не очень понял почему загрузка данных из файла - не ответственность репозитория?
    Если, например, данные берутся из базы - то репозиторий ищет запись и возвращает объект-сущность.
    В данной задаче вместо базы используется файл. Т.е. репозиторий по сути вместо базы обращается к данным из файла (только чтение, никакой записи) и также возвращает объект-сущность.
  • Может ли кто-то проревьюить ООП код на PHP (тестовая задача, Symfony)?

    @Anton_O_B Автор вопроса
    Daria Motorina Спасибо за столь развернутый ответ.
    2) private методы поместил здесь для читабельности. Опять же руководствовался "Чистым кодом" Р. Мартина, там рекомендуется их размещать сразу после методов, которые их в первый раз используют. Такой же порядок организован по всему коду.
    6) могли бы пояснить подробнее, почему именно, например, Builder и остальные классы в подпапках Service не являются сервисами в данном случае? Я к тому, что если в данном случае рассматривать с точки зрения domain model, где классы разделяются на Entity, Value и Service, то, например тот же Builder не является Entity, и не является Value. Получается, что это сервис. Или не так?
    9) Director - часть шаблона Builder (один из вариантов его использования). Т.е. в директоре можно управлять шагами Builder. (вызывать не все шаги или, например, менять их порядок) .Здесь в директор я добавил логическую валидацию параметров запроса, поиск Employee в репозитории и директор решает, нужно ли выполнять все шаги Builder. Мне думается, что это и есть ответственность директора.
    По поводу наименования класса, опять же по "Чистому коду", рекомендуется включать общеупотребительные технические термины в имена классов - например, Builder или Director. Manager, по-моему, к таковым не относится, а для Service классов есть отдельный каталог.
    Кроме того, мне показалось логичным использовать здесь класс директора еще и с точки зрения модели предметной области - аналогия с реальным директором, который принимает какие-то параметры запроса, находит нужного работника и далее делегирует всю работу по вычислению расписания.
    10) подчеркивания использовал для обозначения переменных, которые в тестовом методе не используются - в данном случае в провайдере тестовых данных, есть несколько ключей, и получается, что их надо в параметрах метода указать все, чтобы добраться до нужного или разбивать провайдер на несколько. Согласен, что выглядит странновато, но, например, в других языках программирования такая практика применяется, т.е. если параметр _ - то он как бы есть, но как бы не нужен.
  • Может ли кто-то проревьюить ООП код на PHP (тестовая задача, Symfony)?

    @Anton_O_B Автор вопроса
    FanatPHP Я к тому, что если, например, формат выдачи ошибок должен быть пользовательским - т. е. если, например, json с сообщениями об ошибках должен быть определенного вида - то получается нужно будет также добавлять класс подписчика на Symfony Exception и форматировать ответ уже при возникновении Exception. И это по сути такой же вариант, что и с оберткой, с поправкой на то, что надо будет исключения, которые возникли при работе с файлом фильтровать прямо в методе подписчика.
    Или не так?
  • Может ли кто-то проревьюить ООП код на PHP (тестовая задача, Symfony)?

    @Anton_O_B Автор вопроса
    @Compolomus
    "Запросы к стороннему апи надо кэшировать, чтоб постоянно не дергать календарь"
    Я вроде бы кэширую запросы через Proxy pattern
    т.е. в классе Calendar есть свойство, которое сохраняет результат, а запрос идет только если в свойстве значения нет.
    Или имеете ввиду более долгосрочный кэш?
  • Может ли кто-то проревьюить ООП код на PHP (тестовая задача, Symfony)?

    @Anton_O_B Автор вопроса
    FanatPHP я видел где-то пример, может, конечно, что-то путаю, что обертка для Exception вроде как более user friendly. Т.е. как бы можно залоггировать служебное сообщение, а пользователю отправить красивый текст. Я из этих соображений.
  • Может ли кто-то проревьюить ООП код на PHP (тестовая задача, Symfony)?

    @Anton_O_B Автор вопроса
    FanatPHP т.е. по сути не нужен try catch для чтения файла?
  • Может ли кто-то проревьюить ООП код на PHP (тестовая задача, Symfony)?

    @Anton_O_B Автор вопроса
    Дмитрий - по поводу кучи интерфейсов: да, для каждого сервиса создан свой интерфейс. А это вроде бы к D относится из аббревиатуры SOLID. Т.е. как бы зависимости между сервисами устанавливаются на уровне интерфейсов, а не конкретных классов.
  • Может ли кто-то проревьюить ООП код на PHP (тестовая задача, Symfony)?

    @Anton_O_B Автор вопроса
    imdeveloper да, PHP doc здесь не используется. Прочитал книгу "Чистый код" Р. Мартина - там вроде как рекомендуется использовать PHP doc только для публичных API, а весь внутренний код должен быть самодокументирован - с помощью типизации, именования - исходил из этого.
  • Может ли кто-то проревьюить ООП код на PHP (тестовая задача, Symfony)?

    @Anton_O_B Автор вопроса
    FanatPHP, я тут думал, что таким образом (через создание собственного типа исключения UnableToLoadEmployeeDataException) получается обертка над всеми исключениями, которые могут произойти при чтении файла. И далее в подписчике на событие исключения проверяем тип и ставим в ответ 500.
  • Может ли кто-то проревьюить ООП код на PHP (тестовая задача, Symfony)?

    @Anton_O_B Автор вопроса
    Дмитрий,
    Аргументы в тестах используются, потому что используется data provider. Это не специфично для Symfony, а идет из PHPUnit. Но в целом, это идея - вместо того, чтобы передавать все аргументы из одного провайдера, можно разделить на несколько провайдеров и как-то их сгруппировать, чтобы в тестовых методах не передавать неиспользуемые параметры.
    Можно, конечно, и без аргументов/провайдеров, т.е. просто писать данные в блоке Arrange для каждого теста, но тогда будет дублирование.
    А нечитаемые - это вы имеете ввиду само тело теста или название метода?
  • Может ли кто-то проревьюить ООП код на PHP (тестовая задача, Symfony)?

    @Anton_O_B Автор вопроса
    Дмитрий, FanatPHP, imdeveloper прошу прощения, что не сразу ссылку.
    Код здесь:
    https://github.com/ab2021-dev/test-task
    Нужно именно ревью, что плохо в данном коде. Писал сам, несколько раз рефакторил.
  • PHPUnit сообщения об Exception в консоли - нормально ли?

    @Anton_O_B Автор вопроса
    Антон Шаманов, точно, спасибо. У меня ошибка была в том, что я создавал экземпляр объекта-заглушки перед вызовом метода с исключением. А createMock уже возвращает экземпляр.
  • PHPUnit сообщения об Exception в консоли - нормально ли?

    @Anton_O_B Автор вопроса
    Могли бы показать какой-нибудь рабочий пример с этой конструкцией?

    Я пробую так:
    namespace App\Tests\Controller;
    
    use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
    use Exception;
    
    class CustomException extends Exception
    {
    
    }
    
    class Demo
    {
        public function some()
        {
            echo '123';
        }
    }
    
    class SomeControllerTest extends WebTestCase
    {
        public function testDemo()
        {
           $this->expectException(CustomException::class);
           $demoStub = $this->createMock(Demo::class);
           $demoStub->method('some')->willThrowException(new CustomException('Exception from some method.'));
           $demoIntance = new $demoStub();
           $demoIntance->some();
        }
    }

    Но выдает:
    Failed asserting that exception of type "App\Tests\Controller\CustomException" is thrown.

    Т. е. как бы получается, что метод some должен выбрасывать исключение, но либо этого не происходит, либо оно не перехватывается с помощью $this->expectException.
  • PHPUnit сообщения об Exception в консоли - нормально ли?

    @Anton_O_B Автор вопроса
    Сергей Романенко,
    У меня получается так - тест пройден. Но, судя по консоли, Uncaught Exception в строке с new MyException.
    Выглядит странно. Получается, что метод отрабатывает корректно при вызове из контроллера, но при этом еще кидается исключение прямо в код теста, которое не перехватывается стандартным для этого способом
    $this->expectException();
    У меня есть предположение, что эти сообщения в консоли можно просто проигнорировать (все-таки это всего лишь тесты и возможно это нормальное поведение для PHPUnit и данной ситуации).
    И есть подозрение, что так лучше не делать (возможно закралась какая-то ошибка в логике обработки исключений в рабочем коде - хотя там вроде все прозрачно и просто, или, например, я неправильно тестирую данную ситуация - хотя тут тоже выглядит логично).