Ответы пользователя по тегу Code review
  • Может ли кто-то проревьюить ООП код на 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 комментариев
  • Что мне изменить в этом проекте?

    glaphire
    @glaphire Куратор тега PHP
    PHP developer
    1) в репозитории у Вас получился скорее скелетон, а не пакет фреймворка, у папки шаблонов должно быть осознанное предназначение, нп. приветствующая страница, папка public тоже обычно отсутствует
    2) именование интерфейсов как iSomething морально устарело и неправильно по PSR (для классов тоже, там был класс reCaptcha)
    3) тесты должны отражать тесты на функционал фреймворка, пока там заглушки
    4) непонятен слой Promises, под ним там видно много классов, которые отвечают за разные вещи, стоило бы их структурировать, приставка extreme тоже не отражает предназначение классов
    5) странно видеть в core класс рекапчи, это типично клиентская задача и фреймворк не должен ее решать
    6) сейчас есть тенденция подключать сторонние orm во фреймворк, потому что проектирование абстракций для базы с нуля это огромная работа
    Ответ написан
    2 комментария
  • Можете поревьюить?

    glaphire
    @glaphire Куратор тега PHP
    PHP developer
    1. ссылка - не экономьте на autoloader, это не рационально, если уже начали использовать :)
    2. ссылка - лучше собирать такие строки заранее в переменную и потом передавать как аргумент, неудобно поддерживать их "внутри скобок", а экономия на одной переменной ничего не даст
    3. ссылка Структура респонса никак не описывается, т.е. нельзя ничего понять, пока не посмотришь в доку vk или руками не потестишь. Нужно или инкапсулировать респонс в класс, или дать ему какую-то предсказуемую структуру массива.
    4. ccылка Внести больше ясности в "$key - 1" не описанием в phpdoc, а рефакторингом в максимально очевидный код.
    5. ссылка Нет гарантии, что к элементу $messages[0] можно применить метод out, надо это как-то предотвращать.
    6. ссылка конструктор затерялся среди множества методов, хорошо бы его вынести в начало :)
    7. ссылка хорошим тоном считается выносить в константы любые числа, даже очевидные (60).

    UPD.
    1. Общее замечание - на входные параметры методов тоже надо typehints писать.
    2. Много комментариев к коду - это признак того, что много неочевидных шагов, значит их надо дробить, инкапсулировать в методы, может даже выносить в отдельные классы, чтобы не перегружать текущий.
    Ответ написан
    Комментировать