«phpClub» — архив тем ("тредов"), посвящённых изучению PHP и веб-технологий.
Аноним 2019/01/16 16:02:44  №1328089 1
Ответы: >>1328512 >>1329271 >>1331382
Аноним 2019/01/21 02:57:50  №1331382 2
Напомню себе проверить: 16/01/19 >>1328089 https://github.com/asdasdasdasddasasdasdas/StudentList

Кроме этого, я в прошлом треде ответил на все вопросы, проверил все задачки. Зайдите и посмотрите, если вы что-то спрашивали: >>1305368 (OP)

Если вас пропустили, напомните о себе в этом треде.
Ответы: >>1333958 >>1339036 >>1339037
Аноним 2019/01/24 11:56:23  №1333958 3
>>1331382
Проверь пожалуйста. Мне очень надо.
https://github.com/asdasdasdasddasasdasdas/StudentList Аноним 2019/02/01 00:45:59  №1339036 4
>>1331382

> https://github.com/asdasdasdasddasasdasdas/StudentList/blob/12042279530b7358e443fdb759db3083a3e1cc1d/users.sql#L32
> `hash` varchar(100) NOT NULL,
Здесь бы не помешал комментарий к колонке (COMMENT '...'), что это за хеш.

> groupa
Это и не английское слово, и не транслит с русского. Не стоит выбирать такие идентификаторы, так как при работе с кодом ты будешь делать ошибки в написании, и тратить время на их исправление. В идеале надо использовать английский язык или хотя бы транслит.

> `gender` varchar(1) NOT NULL
Тут бы лучше подошел тип ENUM

> `email` varchar(20)
Я бы сделал побольше символов.

> CHARSET=utf8;
Кодировка utf-8 обозначается в MySQL как utf8mb4. utf8 - это урезанная кодировка (utf8mb3), я бы не советовал ее использовать, так как в ней, например, нет кодов эмодзи.

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

Сейчас получается, любой может скачать SQL-дамп с данными пользователей.

> https://github.com/asdasdasdasddasasdasdas/StudentList/blob/12042279530b7358e443fdb759db3083a3e1cc1d/index.php#L6
Здесь в автозагрузчике получается относительный путь вроде "Some/file.php". Этот путь будет интерпретироваться относительно текущей директории процесса (не обязательно относительно папки с файлом index.php), а также искаться во всяких include_path. Лучше составить полный путь, используя константу __DIR__.

> https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/app/bootstrap.php
Конфиг лучше бы вынести в отдельный файл, который находится в gitignore (это вроде описано в комментариях к задаче). А то сейчас, если один разработчик впишет туда свой пароль и закоммитит изменения, то эти изменения появятся у других разработчиков.

> https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/app/core/Router.php

Здесь пока плохо с форматированием. Тебе надо либо приучиться сразу писать код в правильном стиле, либо использовать какую-то IDE c встроенным форматированием. Например, Netbeans, PhpStorm, VS code (могут понадобиться плагины). Вот пример, обрати внимание на пробелы вокруг знака равно и запятой:

> $params = trim(parse_url($_SERVER["REQUEST_URI"], PHP_URL_PATH),"/");
> $params =explode('/',$params);

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

Далее, тут:
> if($params[1]) {

Если в массиве нет элемента с индексом 1, то будет ошибка (notice или warning). Надо проверять через array_key_exists, empty или isset.

> https://github.com/asdasdasdasddasasdasdas/StudentList/blob/12042279530b7358e443fdb759db3083a3e1cc1d/app/core/Router.php#L33
> try {
> } catch (\Exception $e) {

Здесь скорее всего неправильно исопльзуются исключения. Если ты хочешь ловить и обрабатывать исключения, тебе надо сделать свой класс исключений для каждого вида ошибки. А так, ты ловишь вообще все исключения в программе, и вместо записи их в лог (что PHP делает по умолчанию, если ты их не ловишь) просто выводишь на экран, не записывая в лог. Почитай урок https://github.com/codedokode/pasta/blob/master/php/exceptions.md

Также, для ошибки "страница не найдена" надо отдавать HTTP код 404. Если ты не знаешь, что такое HTTP и код статуса, почитай что-нибудь по этой теме, например урок https://github.com/codedokode/pasta/blob/master/network/http.md

> https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/app/controller/ControllerFactory.php

Нельзя ли создание контроллеров делать в DI контейнере? А то получается дублирование тут его функционала. Правда, контроллер надо каждый раз создавать новый, а твой DI контейнер, увы, так не умеет. Если это слищком сложно, можно оставить так, но можно бы и попробовать сделать. Для этого надо в DI контейнер передавать не созданные объекты, а функции для их создания, и он будет вызывать их при необходимости.

https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/app/controller/controller.php
Имя файла должно совпадать с именем класса с учетом регистра. У тебя файл с маленькой буквы. Под Windows, чтобы поменять регистр букв в имени, может понадобиться переименовать файл под другим именем сначала.

> https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/app/controller/MainController.php
> private $model;
Лучше бы давать более понятное название, например, studentTG или как-то так. Так код будет понятнее.

> $search=$_GET['search'];
Лучше делать strval и trim. Так как могут передать массив вместо строки, а также, там могут быть лишние пробелы.

> setCurrentPage($_GET['page']);
Здесь тоже безопаснее принудительно преобразовать данные от пользователя к числу.

> paginator->countPage
Неточное название, лучше setRecordCount или setItemCount, или как-то так.

> https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/app/controller/ProfileController.php
> https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/app/controller/RegistrationController.php

Тут много общего кода. Лучше всего было бы объединить его, а не копипастить.

> https://github.com/asdasdasdasddasasdasdas/StudentList/blob/89c01482b1e548fb1b258bf2b7708aa687911571/app/controller/RegistrationController.php#L21
> if(!$this->auth->checkHash())
А что, если пользователь залогинен? Выводим белую страницу? Надо эту ситуацию тоже обрабатывать.

> header("Location:/profile");die();
Тут наверно лучше использовать return, вдруг ты захочешь в index.php или в роутере что-то делать после обработки запроса. Хотя можно пока и так.

> if($this->auth->checkHash())
> {
> $student = $this->model->getStudentByHash($_COOKIE['hash']);
Тебе не кажется, что правильнее было бы инкапсулировать (скрыть) всю работу с куками в AuthService и сделать в нем готовый метод для получения текущего пользователя? А то у тебя нет единого ответственного за работу с кукой hash, вместо этого работа с кукой размазана по всему коду. Плохо.

> public function __construct($model,$validator,$auth)
Здесь нужны тайп-хинты для защиты от ошибок и повышения понятности кода.

> private function grabPostValues()
Здесь бы нужен тайп-хинт на возвращаемое функцией значение.

https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/app/helpers/Authorization.php
Названия функций можно улучшить, также стоит добавить тайп-хинты на возвращаемые значения функций.

Вместо checkHash лучше написать что-то вроде isLoggedIn().

> https://github.com/asdasdasdasddasasdasdas/StudentList/blob/89c01482b1e548fb1b258bf2b7708aa687911571/app/helpers/DIContainer.php#L35
Тут правильнее было бы сделать свой класс исключений, если уж делать все идеально. А то как ловить такое исключение и отличать от других?

https://github.com/asdasdasdasddasasdasdas/StudentList/blob/89c01482b1e548fb1b258bf2b7708aa687911571/app/helpers/DIContainer.php#L9
> public $dependencies = [];
А почему public? Думаю, тут лучше ограничить доступ к полю. Ты не читал про инкапсуляцию?

---------

Инкапсуляция. У этого слова есть разные определения, в том числе такие что ничего не понять, потому объясню простыми словами.

Суть инкапсуляции в том, что класс скрывает (инкапслирует) в себе логику работы с данными и сами данные, а наружу выставляет только методы. Пользователю этих методов не важно, как класс устроен внутри, как он хранит данные, какие у него есть поля, ему достаточно вызвать нужный метод чтобы получить результат.

Кроме нескольких публичных методов, остальные методы и свойства закрываются от доступа снаружи модификаторами private или protected. То есть с объектом снаружи ничего нельзя сделать, кроме вызова публичных методов.

Это упрощает понимание кода: тебе не надо читать и разбирать код класса, достаточно прочитать название публичных методов (и может быть комментарии к ним). Также, это упрощает изменение кода: если какое-то свойство имеет уровень private, то доступ к нему возможен только из того же класса и тебе не надо бегать по всему коду и смотреть что там с этим свойством делается, тебе достаточно просмотреть один файл с этим классом.

При инкапсуляции автор класса таким образом задает ограничения, что можно делать с объектом.

Как плюс, мы можем поставить какие-то проверки в методах, и запретить установку неправильных значений свойств. Таким образом, снаружи записать неправильное значение в объект будет нельзя и автор класса может гарантировать его корректную работу в любой ситуации. Если у нас есть публичные свойства, то в них можно записывать что угодно, а приватные свойства изменять снаружи нельзя, можно только вызвать методы, которые что-то делают.

Инкапсуляция это хорошо. Так как весь код, который занимается одной задачей, оказывается заключен внутри одного класса. Противоположный случай это когда код (или знание о его внутреннем устройстве) вылезает из класса и размазывается по всей программе.

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

---------

https://github.com/asdasdasdasddasasdasdas/StudentList Аноним 2019/02/01 00:46:38  №1339037 5
>>1331382

> public function countPage($cstudents)
Тут нужен тайп-хинт и надо бы переименовать функцию.

> https://github.com/asdasdasdasddasasdasdas/StudentList/blob/89c01482b1e548fb1b258bf2b7708aa687911571/app/helpers/Paginator.php#L26

> return "/?page=".$previousPage.'&search='.$_GET['search'];
Вот это плохо. Почему класс лезет куда-то в параметры запроса? Откуда он знает, что там хранится параметр поиска? У тебя нет разделения ответственности между пагинатором и контроллером. Не надо вообще в GET/POST лезть откуда-то, кроме контроллера.

Вдобавок, ты еще и неправильно подставляешь данные в ссылку, не исплоьзуя процентное кодирование спецсимволов через urlencode. А что, если там встретятся символы & или # ?

Наконец, гибче было бы, если бы пагинатор вообще не знал про поиск, а просто принимал дополнительные параметры для ссылки.

И еще. Сейчас у тебя некоторые методы пагинатора неявно полагаются на то, что ранее были вызваны другие методы. Например, метод setCurrentPage() ожидает, что перед ним будет вызван countPage(), но это никак не проверяется и даже никак не задокументировано. Как другому программисту разобраться в этом? Это признак плохого кода.

Исправить это можно так:

- сделать передачу обязательных параметров в конструкторе, так, что не получится создать пагинатор без их указания
- или сделать проверку на заполнение нужных параметров в фукнции, которая их использует

Первое наверно проще.

> public function ValidateAll($student)
Нужен тайп-хинт, а в идеале еще комментарий. Имена методов пишут с маленькой буквы.

> public function fill($post) :void
> {
> $this->name = $post['name'];
А тут есть гарантия, что в массиве всегда есть такой элемент? Также, нужен тайп-хинт.

> return $stmt->fetchAll(PDO::FETCH_CLASS);
Имей в виду, что у FETCH_CLASS есть странность: http://phpfaq.ru/pdo/fetch#oop

> По умолчанию PDO присваивает значения свойствам класса до вызова конструктора. При помощи же данной константы это поведение можно изменить - сначала будет вызываться конструктор

Это реально странно, так как конструктор в теории может сбрасывать значения свойств.

> public function countAllStudent() : int
> return $result->fetchColumn();
fetchColumn() возвращает строку, а не число. Ты увидишь ошибку, если включишь strict_types=1 в начале файла.

https://github.com/asdasdasdasddasasdasdas/StudentList/blob/89c01482b1e548fb1b258bf2b7708aa687911571/app/model/StudentTableGateway.php#L133

Здесь ошибка в логике. Если в БД есть совпадающий email, но первой вернется строчка с id пользователя, то это не будет замечено. Перепроверь логику работы функции.

https://github.com/asdasdasdasddasasdasdas/StudentList/tree/ads/public/view
Вообще, view не надо выкладывать в паблик. Их исходный код ведь пользователю видеть не надо.

https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/public/view/layouts/def.php
Форматирование кода очень плохое. Отступы неправильные.

> if(!$this->auth->checkHash()) : ?>

Лучше было бы наверно передавать переменную loggedIn.

> href=<?=$this->paginator->getPreviousPageUrl();?>
А где экранирование? Не забывай, что & это спецсимвол в HTML.

> https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/public/view/partials/form.php
Форматирование плохое.

> <?= $errors['surname_error'] ?>
Тут лучше бы экранировать все на всякий случай.

> <input class="form-control " type="number" name="balli" value=<?=htmlspecialchars($student->balli) ?> required>

А нельзя ли тут ограничить мин/макс значения?

> <?= $student->gender == 'f' ?
Лучше бы конечно использовать константу.

> html,body{
> height:1000px;
> overflow-y: hidden;
> overflow-x: hidden;
По моему, это плохая идея. Если что-то не поместится на экран, то это будет никак не увидеть.

> thead th{
Ты зря задаешь стили для всех таблиц на сайте. Лучше применять только к таблицам с определенным классом. Это тебе потом помешает при развитии сайта.

> form{
То же самое.

> span.text-danger{
То же самое.

> .table-responsive{
То же самое. Сделал бы класс вроде student-table и применял бы к нему.