Решил задачку про список студентов от ОПаhttps://github.com/codedokode/pasta/blob/master/student-list.mdОП, аноны, можете посмотреть ? https://github.com/qqzooi/student-list
>>1960025> UNIQUE KEY `unique_index` (`email`,`token`)Тут наверно надо 2 отдельных индекса - по email и по токену. А то у тебя, получается, можно внести несколько записей с одинаковым email, если у них разные токены.> return json_decode(file_get_contents($file));Советую добавлять флаг JSON_THROW_ON_ERROR, без него при неправильном JSON функция просто вернет null и не выдаст никакой ошибки (см. мануал по json_decode).Также, мне кажется, JSON лучше преобразовывать в массив, а не в объект. Так как stdСlass в PHP - это непонятное недоразумение, это по сути не объект, а просто массив (так как у него не определен список полей и нет методов), только без функций для работы с ним. Зачем stdClass нужен, я не понимаю. Если ты хочешь мапить конфиг на объект, то сделай для этого класс и десериализуй JSON в объект этого класса. Если не хочешь, то наверно лучше будет использовать массив.> $container->register('request'Мне кажется, не очень логично регистрировать в контейнере реквест. Ведь в контейнере обычно регистрируют постоянно живующие сервисы, которые неизменны. Реквест же меняется на каждом запросе. Потому его нелогично класть в контейнер и вдвойне нелогично, если какие-то сервисы от него зависят. Представь, что у тебя неумирающий после каждого запроса сервер (или ты просто решил написать тесты). Захочешь ты обработать несколько разных реквестов и это не получится сделать.У тебя от реквеста зависит роутер. Получается, это такой "одноразовый" роутер, который может обработать лишь один реквест? Нелогично. Роутер логично положить в контейнер, но нелогично, что мы передаем реквест ему в конструктор. Правильнее сделать в нем метод route($request) и тогда один роутер сможет обработать любое количество реквестов. А вот список роутов обычно передают в конструктор роутера. Так роутер будет чуть более универсальным и в нем не будет захардкожен путь к конфигу (и ему вообще не важно будет, откуда берется конфиг).Соответственно, в App мы бы создавали новый реквест и передавали в роутер. А тот передавал бы реквест и параметры роутинга в контроллер.Класс Db не особо нужен. У меня такое ощущение, что в каких-то других учебниках зачем-то учат его делать и потому он появляется в коде в каждой работе. Класс Db (создание объекта PDO) полностью можно перенести в bootstrap.php. Ты ведь для других объектов не делаешь классы-фабрики, а для PDO почему-то сделал. > @var DIContainer> private DIContainer $container;> @param DIContainer $container> public function __construct(DIContainer $container)> @param int $statusCode> @return Response> private function createResponse(int $statusCode = 200): ResponseНе стоит писать phpDoc, если он не несет никакой новой информации, а просто дублирует указанный в коде тип. > error_log($exception->__toString());Ошибки типа 404 можно не логгировать (они все равно логгируются на веб-сервере вроде nginx, если нужно). Насчет исключений, я не думаю, что все исключения должны наследовать ApplicationException. Как я понял, этот класс обозначает исключение, к которому привязан определенный HTTP код и сообщение на странице ошибки. Но если взять тот же ContainerException (или DbException), какое отношение контейнер имеет к HTTP-ошибкам? Никакого. Контейнер не должен решать, какую страницу ошибки выводить и что на ней писать, он должен просто сигнализировать об ошибке и все. Потому он не должен наследоваться от ApplicationException. От него должны наследоваться только исключения вроде ошибок доступа (код 403), ошибки 404 и тд. Получается нарушение принципа единой ответственности. Контейнер или БД, выбрасывая исключение, определяют, какую страницу ошибки показать, с каким кодом и каким сообщением. Это не их ответственность.> $split = $this->getSplitRealPath();> $request->setRequestBody($split);Этот момент в роутере выглядит коряво. Почему-то значения GET/POST проставляются в Request только после роутинга. Логичней проставлять их при создании Request, а позже, при необходимости, добавлять значения параметров через $request->setAttributes. Хотя, мне не очень нравится, что в реквест вообще что-то дописывается. По идее же реквест представляет пришедший от пользователя запрос, а нем нет никаких атрибутов. В нем есть URL, GET, POST и все. Я знаю, что некоторые фреймворки дописывают атрибуты в реквест, но это, на мой взгляд, выглядит коряво. Логичнее было бы параметры после роутинга не записывать в реквест, а просто передавать в контроллер. Из-за этого у тебя Request и Router получаются спутаны друг с другом, а лучше бы если они были независимы. Уж Request точно не должен никак зависеть от роутера.actionIndex и actionSearch можно было бы объединить вместе, это очень похожие вещи - выборка из таблицы.> private function getHtml(string $items): stringЭто конечно корявенько, что HTML формируется не через шаблон. Можно было сделать, чтобы Pagination просто возвращал бы данные для отображения пагинатора, а HTML формировать в шаблоне из этих данных.Неаккуратно сделано, что CookieHelper работает в обход Request/Response и ставит куки напрямую. > public function getCsrfToken(): string> return ($this->cookie->getCookie('csrf')) ?: $this->csrfToken;Логика работы этой функции непонятна. Она возвращает непонятно что.> if ($_SERVER['REQUEST_METHOD'] == 'POST') {Это логичнее делать через $request->isPost().> private int $outputRows;Мне кажется, это стоило просто прописать в контроллере. StudentTableGateway не отвечает за отображение данных и не знает, по сколько строк надо выводить.В функции getAll() наверно не надо было заморачиваться с джойном. Обычно делают так, что пагинация работает после сортировки, то есть сначала записи сортируются и затем из них выбирается запрошенная страница. Просто ORDER BY ... LIMIT ....Не очень хорошо, что разные функции возвращают объекты Student с разным набором полей. Это может вызвать потом путаницу в коде, так как непонятно, как различать эти объекты. Что, если ты передашь "неполноценный" объект в функцию, которой нужно какое-то поле, которого в нем нет? В роутинге не стоило указывать в регулярках GET-параметры. Ведь никто не гарантирует, что они придут именно в таком порядке, как описано: > ?key=(id|name|surname|sgroup|score)&sort=(asc|desc)(&page=[1-9][0-9])?Обычно при роутинге GET-параметры игнорируют и смотрят только на path.> (isset($student->gender) && $student->gender == 'male'Тут стоило использовать константу, наверно.В общем, сделано хорошо. Видно, что ты поковырялся в сторонних фреймворках.Советую также сделать описание в README и добавить скриншоты, если ты захочешь потом показывать эту работу в резюме.
>>1960646> Из-за этого у тебя Request и Router получаются спутаны друг с другом, а лучше бы если они были независимы.Меня этот момент тоже напрягал, но что-то было лень переписывать, ибо пару раз приходилось переписывать часть логики, т.к. заранее не продумал + не так много опыта, а хотелось побыстрее закончить> Это конечно корявенько, что HTML формируется не через шаблон.Ты имеешь в виду, чтобы сделать отдельный шаблон для пагинатора, а сам пагинатор просто будет возвращать <ul> с необходимыми элементами ? > В функции getAll() наверно не надо было заморачиваться с джойном. Как раз и сделал через джойны для пагинации. Чтобы происходила сортировка только для тех данных, которые отображаются на странице. Если делать без джойнов, то получится, что сначала выполняется сортировка, а только потом LIMIT X, Y. Данные на N странице одни, а после сортировки другие. Как по-другому сделать (если вообще можно по-другому сделать на стороне бека) - не знаюСпасибо большое, Анон, что указал на косяки и что можно исправить. Как думаешь, можно уже пытаться по собесам походить ?
>>1960646> Этот момент в роутере выглядит коряво. Почему-то значения GET/POST проставляются в Request только после роутинга.Вспомнил, почему так вообще получилось. Получилось так из-за того, Request разбирает не только GET/POST, но еще и может разбирать параметры из маршрутов, н.р маршрут: user/(id/[0-9]+) => user/actionView/$1Соответственно, чтобы разобрать параметры, которые заданы через маршрут, ему нужно дергать метод роутера (getSplitRealPath), который как раз получает путь user/actionView/$1 на основе запрошенного маршрута. По итогу придется либо отказываться от этого, либо будут повторяющиеся методы в разных классах, ну или зависимость
>>1960740> Ты имеешь в виду, чтобы сделать отдельный шаблон для пагинатора, а сам пагинатор просто будет возвращать <ul> с необходимыми элементами ?Можно сделать так: делаем отдельный HTML-шаблон и в него передаем объект Paginator. Шаблон выводит HTML, а за подробностями (какие цифры и ссылки выводить) обращается к Paginator.Генерация HTML в коде плохая идея, так как такую верстку трудно поддерживать, разбираться в ней, вносить правки.> Если делать без джойнов, то получится, что сначала выполняется сортировка, а только потом LIMIT X, Y. Данные на N странице одни, а после сортировки другие. Как по-другому сделать (если вообще можно по-другому сделать на стороне бека) - не знаюТак и должно быть, что данные другие. Представь, что ты смотришь первую страницу. Жмешь на сортировку по баллам и видишь тех, у кого больше всего баллов. Жмешь на сортировку по имени и видишь тех, кто раньше по алфавиту. А в твоей системе непонятно как найти тех, у кого больше всего баллов.И еще, обычно при смене сортировки пагинация сбрасывается на первую страницу. > Как думаешь, можно уже пытаться по собесам походить ? Я не HR. Посмотри требования к вакансиям. Согласись сделать тестовое задание, возможно оно по сложности будет как эта задача или проще. >>1960782> Получилось так из-за того, Request разбирает не только GET/POST, но еще и может разбирать параметры из маршрутов, н.р маршрут: > user/(id/[0-9]+) => user/actionView/$1Это $1 в Симфони называют "атрибутами" и роутер после разбора URL помещает их в $request->attributes. Ты бы мог тоже сделать в реквесте массив для атрибутов и методы get/set для него. Но мне кажется, это коряво. Получается, объект Request знает что-то о роутинге, хотя он должен лишь представлять полученные от пользователя данные. Было бы корректнее передавать атрибуты вроде $1 в метод контроллера, например так: public function actionView(Request $r, array $attributes)Или даже так, с использованием рефлексии по имени параметра определять, что туда передать: public function actionView(Request $r, int $id)В Симфони атрибуты реквеста используются и для других вещей - например, код может до вызова контроллера определить город пользователя и сохранить в атрибутах. А контроллер его оттуда возьмет. Но опять же, мне кажется, это коряво, а реквест из-за атрибутов превращается в свалку нетипизированных данных.
>>1961264> Так и должно быть, что данные другие. Понял. Но как ты смотришь на то, чтобы подзапрос в виде джойна оставить ради оптимизации лимита ? Читал статью (на хабре вроде), что если данных очень много и у нас будет LIMIT 100000, 10 например, то это сильно бьет по производительности. В плане того, что mysql сначала переберет 100000 записей, а только потом выдаст следующие 10> Это $1 в Симфони называют "атрибутами" и роутер после разбора URL помещает их в $request->attributes. После прошлого ответа еще посидел подумал и в голову похожая идея пришла. В плане того, что раз это параметры маршрутов, то и разбирать их должен непосредственно сам роутер, а не реквест. Значит в правильную сторону мыслил
>>1961327> Но как ты смотришь на то, чтобы подзапрос в виде джойна оставить ради оптимизации лимита ?Такая проблема производительности лимита есть. Она решается отказом от номеров страниц и использованием индекса, как описано тут https://use-the-index-luke.com/sql/partial-results/fetch-next-pageТо есть, вместо LIMIT 10000, 10 мы используем, например, WHERE id > :lastId LIMIT 10. У тебя сделать это не получится, так как нам надо сортировать по произвольному полю, а не по заранее выбранному.У тебя сортировка сейчас, как мне кажется, сделана некорректно. При смене сортировки результаты на странице (например, первой) должны меняться, а у тебя они не меняются.
>>1960646Анон, я пофиксил. Можешь еще раз посмотреть ? https://github.com/qqzooi/student-list>>1962175>У тебя сортировка сейчас, как мне кажется, сделана некорректно. Да, я уже исправил. Просто изначально думал, что должны сортироваться только те данные, которые в данный момент юзер видит на конкретной странице.
>>1962389В роутере сделаны неудачно, на мой взгляд, методы getController() и getAction(). Чтобы они вернули значение, надо сначала вызвать route(), но как об этом догадаться? Получается, один метод при вызове влияет на работу других методов, это называется побочные эффекты и это нехорошо. Правильнее было бы, если бы был метод, который получает, например, Request или просто $urlPath и возвращает controller, action и attributes.Та же проблема с пагинацией. Было бы лучше, если бы все методы работали корректно без необходимости вызывать метод run().> private int $limitStudents = 10;Это можно было сделать private константой.Плохо, что тут идет вперемешку PHP и HTML: https://github.com/qqzooi/student-list/blob/6466cbedf7026440a2885c871cc93701a0581cce/App/Components/Navbar.php#L7Лучше было бы, если бы Navbar только хранил информацию о пунктах меню, а преобразование в HTML происходило бы в шаблоне. То есть, HTML-шаблон бы обращался к классу Navbar, получал список пунктов и рендерил их. В твоем решении править верстку неудобно, так как она раскидана кусками по классу.То же касается NotificationUtil и других мест, где есть HTML в классах. HTML-код лучше формировать в шаблоне.SVG-код, наверно лучше поместить в файл и подключать через file_get_contents(). Как в твоем коде редактировать иконки? Руками переносить в файл, потом обратно в код? Я думаю, что JOIN в getAll() не нужен, так как он ни на что не влияет.> user-scalable=no, Это в общем плохое решение, так как лишает пользователя удобства. Может, у человека шрифт слишком мелкий или наоборот, мало информации на экран влезает. Я на десктопе часто на сайтах меняю масштаб.
