>>1958592

Ты зачем-то создаешь переменные, которые никак дальше не используются. Вот смотри:

function x() {
return $a = "текст";
}

Эта строка создает переменную $a, записывает в нее значение "текст", затем возвращает его и выходит из функции. При выходе из функции переменная $a уничтожается. Зачем ее тогда было создавать? Нужно писать просто:

return "текст";

У тебя команда

return $errors[] = "текст";

создает массив, но возвращает она не массив, а строку "текст". Зачем создается массив, непонятно, так как он никак не используется и уничтожается при выходе из функции. Если ты хотел вернуть массив из 1 элемента, то надо писать так:

return ["текст"];

Хотя правильнее было бы сначала собрать все ошибки в массив и в конце функции вернуть его:

$errors = [];
$errors[] = "текст";
$errors[] = "текст";
return $errors;

Также, функции надо называть осмысленно. Не form(), а check_form(), validate_form().

>>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 и добавить скриншоты, если ты захочешь потом показывать эту работу в резюме.

>>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)

В Симфони атрибуты реквеста используются и для других вещей - например, код может до вызова контроллера определить город пользователя и сохранить в атрибутах. А контроллер его оттуда возьмет. Но опять же, мне кажется, это коряво, а реквест из-за атрибутов превращается в свалку нетипизированных данных.

>>1961059

> $n%100

это встречается много раз, нужно было сделать переменную "последние 2 цифры" или "десяткоединицы".

> $j = 10 * (floor(( $n%100) / 10));

переменную надо было назвать "десятки". $tens, например.

> $iSpelling

Тоже неудачное название.

> }

> else {

Что за нестандартное форматирование? Во втором посте треда есть ссылка на сайт, где можно отформатировать код.

> $k=null;

Должно быть $k = 0 так как дальше $k сравнивается с числам (if ($k>0)), а null сравнивать с числом неправильно.

Код функции small_number_to_text можно упростить, если вместо переменных $iSpelling, $jSpelling и тд добавлять слова в массив. Тогда не нужны будут ветки, где переменным присваивается null.

> $smallNInText

Лучше назвать $result или $spelling.

> if ($j>=10 and $j<=20) {

> $wordForm = $form3;

Можно избавиться от $wordForm и сразу писать return $form3. Код будет проще.

> if ($partsArray[2])

Так нельзя проверять наличие элемента, так как если его нет, то обращение [2] вызовет ошибку. Надо проверять через array_key_exists или count($x) >= 2

> $partsArray

Надо было назвать $groups или просто $parts.

В общем, тебе надо сделать следующее:

- поменять названия переменных
- отформатировать код по правилам
- попробовать его укоротить, как я описал выше (не теряя читабельности. Не надо пытаться запихнуть несколько строчек в одну).

>>1961349

> В общем, лучше всего давать переменным имя, которое максимально будет ее описывать, если это возможно, так?

Имя должно описывать, что хранится в переменной. При этом желательно кратко.

$millionWord = getWordForm($millions);
$words[] = $millionWord;

это читается как:
слово для миллиона = выбрать форму слова для миллионов
добавить в массив "слова" "слово для миллиона".

Еще лучше убрать лишнюю переменную и написать сразу:

$words[] = getWordForm($millions);

А если написать

$x = $spelling[$j];

то это не читается.

Представь, что ты открываешь проект в 100 000 строк кода (который ты никогда не прочтешь целиком), и в нем какую-то функцию и в ней какую-то строчку. Ты хочешь сразу понять, что она делает, а не искать выше что это за переменная $j и что в ней хранится. Код с переменными вроде $j нечитабелен. Аналогично с именами функций.

> Но при объединении массива через implode (' ',$x) добавлялись лишние пробелы в числах, в которых отсутствовали сотни, десятки или единицы. Я что-то делаю неправильно?

не добавляй в массив пустые строчки и пробелы не появятся.

ОП, или кто тоже разбирается. подскажите вот в чем:
Есть приложение, просто роутер который вызывает обработчики.
В обработчике/контроллере я хочу что то сделать, допустим записать лог в файл.
Пусть у меня нет сервис локатора, или DI контейнера.
Как я реализую подобное:
допустим метод класса обрабатывает запрос
...
public function anyMethod(){

//создаю фабрику сервисов Логгирования
//которая принимает аргументом конкретный сервис ( MonologLoggerService ), который имплементирует общий для всех логгинг-сервисов интерфейс, что бы методы в контроллерах были одни и те же, и я смог легко поменять один сервис логгирования на другой.

$serviceFabric = new ServiceFabric ( new MonologLoggerService );

$serviceLogger = $serviceFabric->getInstanseLogger();

$serviceLogger->warning('что нибудь пишу в лог');

}

}

Если абстрагироваться от контейнеров и сервис локатора - такой подход адекватен?
Это фактически мои первые потуги в рамках ООП.
Получается в моем варианте фабрика - это не совсем то что должно быть?
Фабрика возвращать должна один и тот же экземпляр одного конкретного класса - просто с разными настройками?
Или фабрика может возвращать экземпляры разных классов , объединенных общим интерфейсом или абстрактным классом?

>>1966063

>>Вот например, зачем ты сделал фабрику логгеров? Что тебе мешает сразу создать логгер через new MonlogLoggerService?

Изначально вообще у был некий общий сервис логгеров, а в нем в конструкторе был захардкоден вызов конкретного допустим MonlogLoggerService. Идея была в том что бы я в общем сервисе логинов мог менять конкретный логгер, а так как в клиентском коде везде вызывается общий сервис - то можно было поменять логер в одном месте. Фактически как в контейнере хранить сервис.
Но прочел что не комильфо в конструкторе определять зависимости и родилась идея передавать конкретный сервис в конструктор.
Более того - если в ходе выполнения программы мне нужно что бы в одном случае один сервис логгинга вызывался, а в другом другой - то вариант захардокоденного вызова конкретного сервиса не даст это сделать в принципе.

>>Бывает и так, и так. Фабрику обычно добавляют для решения таких проблем:

>>Ну и вопрос: чем тебе не подходит DI?

Я в первую очередь решал такую для себя проблему - получить возможность быстрой замены конкретной реализации какого то сервиса во всем клиентском коде в приложении. И я специально не пользуюсь DI - контейнерами, или банальным сервис-локатором. Хочу лучше прочувствовать что дает не правильный подход. Мне как то проще сделать криво, столкнуться со всеми минусами кривого решения, и тогда я по настоящему понимаю реальные плюсы правильного подхода.

>>Не видя кода ServiceFabric, нельзя сказать, зависит он от MonologLoggerService или нет. На первый взгляд, он зависит, так как требует его передачи в конструктор.

А как же в таком случае должен выглядить полностью не зависимый ServiceFabric?

А как поправить мой пример кода так что бы стало более правильно?

И вот вопрос тогда, вот в DI контейнере я определил сервис, LogService::class. И в клиентском коде я получаю этот объект из контейнера. А после я решил что в контейнере LogService::class должен быть иной логгер, не монолог. Я просто руками в контейнере меняю Монолог на что то другое, и все?
Или есть иной вариант.

Сорр если туплю, я Зандстру начал читать , разделы про ООП, пока что все немного в кучу. У него сервис локатор это класс, который через статические методы получает и возвращает какие то объекты - паттерн Регистр примерно так.