«phpClub» — архив тем ("тредов"), посвящённых изучению PHP и веб-технологий.
Аноним 2018/03/30 09:36:59  №1164973 1
Ответы: >>1165603
https://github.com/mlmn/vector.loc Аноним 2018/03/31 10:11:21  №1165603 2
>>1164973

Ой-ой, смесь PHP и HTML, почитай-ка про шаблоны: https://github.com/codedokode/pasta/blob/master/php/templates.md

У тебя очень сложно понять структуру HTML-кода, так как он раскидан по куче функций. И перемежается с кавычками, echo и другим кодом. Такой код очень тяжело редактировать. Изучи шаблоны.

> static public function pageHeader() {
А не надо тут статический метод использовать, зачем? Используй обычный.

> public function addDepartment(Department $dep) {
> if (!in_array($dep, $this->departments, true)) {
При попытке дважды добавить департамент наверно лучше выкидывать исключение. Чтобы сообщить об ошибке.

> if (is_object($employee) and get_parent_class($employee) == 'Employee') {
То же самое, надо сообщать об ошибке, а не тихо ее игнорировать.

> get_parent_class($employee) == 'Employee')
Лучше использовать instanceof

> $info = new stdClass();
Не надо использовать stdClass. Это как массив (так как нигде не описаны его поля), только плохой, так как с ним не работают функции работы с массивами. Тут лучше либо сделать отдельные методы вроде getTotalSalary(), либо специальный объект CompanyStat где описаны все поля.

> public function __construct($name) {
Если ты используешь PHP7, то можно добавить тайп-хинты вроде string, а также тайп-хинты на возвращаемые функцией значения: public function getName(): string

> if (in_array($employee->getName(), $fireList)) {
Мне кажется, лучше удалять не по имени, а по объекту. Так как объект обладает идентичностью и отличается от всех других объектов. И придумывать дополнительные идентификаторы не надо.

> public function makeLeaderByName($name) {
То же самое, не надо придумывать идентификатор, передавай сам объект.

> public function getTopAnalyst() {
Это очень узкоспециальная функция, нужная только антикризисному комитету. Надо ее перенести в антикризисный комитет, а в департаменте сделать универсальный метод поиска по любым критериям.

> if(get_class($employee) == 'Analyst') {
лучше instanceof

> public function demoteLeader() {
нужен ли этот метод? Лучше наверно сделать метод замены босса. А то у тебя можно департамент без босса оставить.

> public function makeLeaderByName($name) {
> public function promoteLeaderByName($name) {
Одинаковые методы же?

> class Names {
Лучше NameGenerator или NameUtil.

Насчет наследовния. У тебя есть негласное правило, что при наследовании профессии от работника надо задать базовые параметры. Но это никак не документировано и никак не проверяется. Легко забыть это сделать. Чтобы этого избежать, можно использовать абстрактные методы - то есть методы, которые не дописаны в базовом классе и которые обязаны реализовать потомки. Попробуй добавить абстрактные методы вроде getDefaultBaseSalary() и тогда их нельзя будет забыть определить.

> class OrganisationBuilder {
> public $org;
> public $dep;
Вообще, эти поля ведь не нужны, вместо них можно использовать обычные переменные.

> class AntiCrisis {
> private $departments;
Мне кажется это поле не нужно, если у тебя есть компания, ты из нее всегда можешь получить департаменты. Получается дублирование данных.

Отбирать работников для увольнения проще так:

- получаем список кандидатов на увольнение
- сортируем его по приоритету (кто в прервую очередь) с помощью usort + анонимная функция
- с помощью array_slice отрезаем нужное число кандидатов
- увольняем

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

Также, надо чтобы программа применила все 3 метода и вывела таблицы для сравнения. Для этого надо научиться делать копии (клоны) организации, чтобы работать с ними, не трогая исходную компанию.
Ответы: >>1173663
Аноним 2018/04/14 17:04:30  №1173663 3
15237146704970.jpg (299, 1280x676)
676x1280
15237146704991.jpg (443, 1920x1200)
1200x1920
15237146705022.jpg (335, 1920x1200)
1200x1920
15237146705043.jpg (157, 1280x720)
720x1280
>>1165603
В общем пришел на пересдачу. Сори что очень долго писал правки.

И так, код: https://github.com/mlmn/vector.loc

Теперь по пунктам с вопросами и комментариями:
1.
>Ой-ой, смесь PHP и HTML...
Не проблема, это просто тянулось еще с тех пор, когда код постился на ideone и нужно было что бы программа была по сути совместимой с консолью, но со временем я запилил себе более удобный вариант для отладки в браузере, который при этом не требовал бы никаких инклудов, что бы быть по прежнему совместимым с ideone, ща консольный вариант выпилил, выпилил шаблоны в отдельыне файлы и перелопатил их.

2.
>> static public function pageHeader() {
>А не надо тут статический метод использовать, зачем?
Вынес header страницы в конструктор Reporter'a, а footer соответственно в деструктор, что бы при создании 1 Reporter'a всегда были все необходимые теги. Как минимум экономится код, жду комментария по этому поводу, предвижу слабое место в том, что кто-то может создавать еще Reporter'ов, но мне решение кажется довольно элегантным, жду оценку.

3.
>При попытке дважды добавить департамент наверно лучше выкидывать исключение. Чтобы сообщить об ошибке.
>То же самое, надо сообщать об ошибке, а не тихо ее игнорировать.
Ок, глянул про исключения, засунул их в места добавления сотрудников и департаментов. А так же отлавливаю в OrganisationBuilder'e. Что бы не копипастить везде
echo 'Выброшено исключение: ', $e->getMessage();
завел статическую функцию для оповещения в Dbg->exceptionEcho($exception).
Только вот у меня вопрос возникает, а для кого пишутся эти исключения? Для самого разработчика, что бы на этапе тестирования увидеть ошибки? Или для пользователя? Останавливать программу после срабатывания такого вот исключения или нет? Если например пытаешься добавить 2 одинаковых департамента, то второй просто не добавляется, но без остановки всё как бы и хорошо работает. Обошли дублирование и всё ок, выполнению программы это не мешает.

4.
>Лучше использовать instanceof
Где мог заменил, в PeopleFactory заменил на is_subclass_of(), так как там проверка не по существующему объекту, а просто по имени класса.

5.
>> $info = new stdClass();
>Не надо использовать stdClass. Это как массив (так как нигде не описаны его поля), только плохой, так как с ним не работают функции работы с массивами. Тут лучше либо сделать отдельные методы вроде getTotalSalary(), либо специальный объект CompanyStat где описаны все поля.
Первый серьезный ступор был тут, и он по сути никуда не делся. Я просто написал класс OrgInfo, но оставил все поля в нем public, так как просто бомбануло при попытке написать в этот класс 24 геттера/сеттера и в том месте где я их заполняю - Organisation->getOrgInfo() тоже просто удлинять код.
Из:
$orgInfo->totalPeople += $dep->countDepEmployees();
делать:
$orgInfo->setTotalPeople() = $orgInfo->getTotalPeople() + $dep->countDepEmployees();
В общем оставил пока такой вот простой объект, и жду комментария что с ним делать, удобный метод getOrgInfo() в котором элегантно всё в одном месте считается через цикл тоже не хочется бить на копипастные методы вида getOrgTotalPeople(), getOrgTotalSalary()...

6.
>> public function __construct($name) {
>Если ты используешь PHP7, то можно добавить тайп-хинты вроде string, а также тайп-хинты на возвращаемые функцией значения: public function getName(): string
До этого писал на 5.х, перекатился на 7.2, вроде расставил везде где нашел тайпхинты как на вход так и на выход. Ну опять же кроме геттеров/сеттеров пустышек. На них тоже нужно выставлять тайпхинт по паравилу хорошего тона, даже если ты просто возвращаешь что-то без изменения/вычисления? Еще не понятно как быть в случае как с моей функцией getTopAnalyst() - либо она возвращает объект Analyst, либо null (по логике отсутствие объекта это как раз оно), и сюда ничего нельзя поставить на выходной тип, будут ошибки.

7.
>> if (in_array($employee->getName(), $fireList)) {
>Мне кажется, лучше удалять не по имени, а по объекту. Так как объект обладает идентичностью и отличается от всех других объектов. И придумывать дополнительные идентификаторы не надо.
>> public function makeLeaderByName($name) {
>То же самое, не надо придумывать идентификатор, передавай сам объект.
Избавился от идентификаторов.

8.
>> public function getTopAnalyst() {
>Это очень узкоспециальная функция, нужная только антикризисному комитету. Надо ее перенести в антикризисный комитет, а в департаменте сделать универсальный метод поиска по любым критериям.
Перенес getTopAnalyst() в класс AntiCrisis. В департаменте завел функцию поиска getAllCertainSpecialists(), в которую нужно было передавать много разных параметров в виде массивов для поиска, поэтому вместо того что бы городить кучу проверок о правильности переданных аргументов просто завел еще 1 класс специально для этой функции - EmployeeSelector. Жду комментарий по поводу этого моего решения.

9.
>> public function demoteLeader() {
>нужен ли этот метод? Лучше наверно сделать метод замены босса. А то у тебя можно департамент без босса оставить.
>> public function makeLeaderByName($name) {
>> public function promoteLeaderByName($name) {
>Одинаковые методы же?
Разобрался, сделал метод замены босса. Но не стал склеивать методы demoteLeader() и promoteLeader() в один большой метод замены, просто заприватил их и оставил для лучшей читабельности. завел публичный swapLeader(), который просто вызывает их в нужной последовательности, логика в этом есть имхо.

10.
> class Names {
ok

11.
>Насчет наследовния. У тебя есть негласное правило, что при наследовании профессии от работника надо задать базовые параметры. Но это никак не документировано и никак не проверяется. Легко забыть это сделать. Чтобы этого избежать, можно использовать абстрактные методы - то есть методы, которые не дописаны в базовом классе и которые обязаны реализовать потомки. Попробуй добавить абстрактные методы вроде getDefaultBaseSalary() и тогда их нельзя будет забыть определить.
Вот тут у меня если честно ступор возник и я вижу дыру теперь, что действительно, если создать новый класс сотрудника, и не прописать в нем типовой конструктор для моего класса, то будет пустой сотрудник без зарплаты, кофе и бумаг. Но как мне поможет абстрактный метод getDefaultBaseSalary(), в классе Employee??
В общем тут у меня проблема и не понимание как из этого слабого места выкрутиться. Не добавлять же абстрактный метод в родителя, который бы вызывался в родительском конструкторе, что бы при наследовании переопределить этот абстрактный метод и типа при переопределении уже в этом методе заполнять стандартные поля значениями (без переопределия как раз ошибка, а при переопределении вроде как и не забудут переписать). В общем я сейчас сяду после этого отчета с этим делом эксперементировать, в гитхаб пока в любом случае не буду коммитить промежуточные варианты.

12.
>> class AntiCrisis {
>> private $departments;
>Мне кажется это поле не нужно, если у тебя есть компания, ты из нее всегда можешь получить департаменты. Получается дублирование данных.
Ок, убрал, но появилось это поле вот так:
Я когда писал этот класс, то заметил что у меня везде в коде копипаста вида:
foreach($this->organisation->getDepartments() as $dep) {
И просто вынес это в отдельное поле да еще и выставляющееся в конструкторе. Не понял если честно что в этом плохого.

13.
>Отбирать работников для увольнения проще так:
>- получаем список кандидатов на увольнение
>- сортируем его по приоритету (кто в прервую очередь) с помощью usort + анонимная функция
>- с помощью array_slice отрезаем нужное число кандидатов
>- увольняем
>Это будет читаться лучше, чем твой код с вложенными циклами и брейками.

Заменил циклы и брейки на usort+анонимные функции. Не имею такого опыта общения с usort, что бы быть уверенным в правильной сортировке сразу по двум параметрам за 1 вызов этой конструкции. Поэтому сначала сортирую пор рангу, потом уже по лидерству.
Функция для инспекции теперь AntiCrisis->prepareEngineersForFire()

14.
>Также, надо чтобы программа применила все 3 метода и вывела таблицы для сравнения. Для этого надо научиться делать копии (клоны) организации, чтобы работать с ними, не трогая исходную компанию.
Ок, 658 и далее строчки теперь посвящены этому, но мне кажется что это не сильно элегантное решение. Может быть стоит антикризис сделать вообще статическим классом? Что бы не создавать под каждую клонированную организацию новый антикризисный объект? А просто передавать ему объект, а он его будет возвращать в измененном виде аки обычная процедурная функция?

Еле еле уместился в 15кб в ответе, пришлось подрезать где можно.
Ответы: >>1176476
https://github.com/mlmn/vector.loc/ Аноним 2018/04/19 07:23:05  №1176476 4
>>1173663

> public function __construct() {
> include 'views/header.php';

Это плохая идея. Конструктор для инициализации объекта, а не для вывода шаблонов.

> public function __destruct() {

Эта идея еще хуже. PHP - язык со сборкой мусора и ты не можешь предсказать, когда будет удален объект и вызван деструктор. Деструктор используется только для освобождения каких-то ресурсов, а лучше по возможности вообще не использовать.

> $orgInfo = $org->getOrgInfo();
> include 'views/reportBody.php';
А зачем нужно OrgInfo? Нельзя ли в организации сделать методы для вычислений этих чисел? Просто у меня ощущение, что этот класс заточен только под вывод таблицы, и тогда код getOrgInfo() желательно вынести из компании куда-нибудь, хотя бы в ту же OrgInfo. Или вообще ликвидировать этот метод.

Поля orgName и orgTitle точно не нужны, так как для них есть методы у компании.

> public function getClass() {
Тут нужен тайп-хинт на возвращаемое значение.

> $this->addEmployee($employee);
> } catch (Exception $e) {
> Dbg::exceptionEcho($e);
Это не нужно делать. try/catch не нужен. PHP сам выводит непойманные исключения, если поменять настройки в php.ini.

> public function promoteStuff(array $promotionList) {
Я не думаю, что это надо делать в департаменте. Тут от департамента ничего не требуется и работников можно повышать напрямую.

> $classMatch = (get_class($employee) == $employeeSelector->getClass());
> $rangMatch = (in_array($employee->getRang(), $employeeSelector->getRang()));
> $leaderMatch = (in_array($employee->isLeader(), $employeeSelector->getLeader()));

По моему было бы гораздо удачнее сделать так:

if ($employeeSelector->matches($employee)) ...

Это позволило бы поместить логику отбора в класс-фильтра, где ей и место. Ну и еще есть такой вариант с коллбеком:

function find...(callable $filter) {
...
if ($filter($employee)) {
$list[] = $employee;
}
...


> public function countDepPageCost(): float {
> return round($pageCost, 3);
Не надо делать тут round, так как это нужно только для вывода в таблицу и должно быть там, где делается вывод, а не тут. Тут должен быть метод, который возвращает точное значение.

> abstract protected function setDefaults();
Это не очень хорошо, так как непонятно, что эта функция должна делать. Это никак не описано и никак не проверяется. Мне непонятно, что в ней надо написать. По моему так лучше сделать функции getStartingSalary(), getStartingCoffee() и тд, с которыми все проще и понятнее.

> public function setLeader($leader) {
Нужны тайп-хинты.

> public function upRang() {
нет проверки на выход за пределы допустимых значений.

> $vectorSecondAC = clone $vectorVanilla;

В клонировании у теябя допущена ошибка. Этот метод не создает глубокую копию компании. он делает клон компании, но в него помещает ссылки на те же самые департаменты с теми же самыми работниками - они по умолчанию не клонируются, а просто копируются ссылки. Изучи магический метод __clone().

> $this->organisation->setTitle("после антикризисных мер #1");
Это неправильно, ради решения задачи про антикризисные меры добавлять в Компанию поле-комментарий. Это не нужно компании, это нужно только в антикризисных мерах, и это надо делать где-то в другом месте.

> new EmployeeSelector('Engineer', [1, 2, 3], [true, false]);
лучше сделать возможность указать "не важно" для ранга. null, например, передать.

Или так:

$selector = new Selector;
$selector->setClass(...);

Сортировку лучше делать в одной функции, так:

$aLeader = $a->isLeader() ? 1 : 0;
$bLeader = $b->isLeader() ? 1 : 0;

if ($aLeader != $bLeader) {
return ...;
}

// иначе, сравниваем ранг

Также, есть еще такой трюк, вычисляем "вес" и сравниваем его:

$aWeight = ( $a->isLeader() ? 100 : 0) + $a->getRank();
$bWeight = ...;

// сравниваем вес

> $managersOfSertainRangs
$managersByRank

> $thisRangToPromote = count($currentRangManagers);
> $neededToPromote = ceil(0.5 * ($thisRangToPromote));
Это лучше записать в одно выражение без промежуточной переменной.

По view: в идеале, надо использовать htmlspecialchars при вставке текста в HTML. Иначе может быть уязвимость XSS: https://github.com/codedokode/pasta/blob/master/security/xss.md

По вопросам:

-------

> Только вот у меня вопрос возникает, а для кого пишутся эти исключения?
Тут есть такой ответ:

Выброс исключения - это способ функции заявить о невозможности выполнения задачи. То есть это такой способ передачи информации out-of-band (не через return). Если исключение планируется ловить, то для этого используется кастомный класс.

То есть функция либо возвращает результат (если он есть), либо выбрасывает исключение. НЕ выбросила - значит, все ок.

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

> Или для пользователя?
Только если чтобы сообщить о них разработчику. Пользователь ведь не разбирается в коде.

Но ты не забывай, что бывает разный пользователь. Есть пользователь программы, а есть пользователь функции - другой разработчик, который ее использует в своем коде.

> Останавливать программу после срабатывания такого вот исключения или нет?
Да. если она неправильная, то ее надо останавливать. Чем раньше ошибка найдена, тем дешевле ее исправить. Читай

- https://habrahabr.ru/post/218325/
- https://www.martinfowler.com/ieeeSoftware/failFast.pdf

> Если например пытаешься добавить 2 одинаковых департамента, то второй просто не добавляется, но без остановки всё как бы и хорошо работает. Обошли дублирование и всё ок, выполнению программы это не мешает.
В программе явно есть ошибка, ты ее скрываешь, в итоге она вылезет где-то на более позднем этапе, например, в виде неправильных чисел. И ее найти будет гораздо труднее. Особенно когда эта программа станет огромной и в ней будут спрятаны сотни разных ошибок, которые сделали предыдущие разработчики, а исправлять придется тебе.

> Первый серьезный ступор был тут, и он по сути никуда не делся. Я просто написал класс OrgInfo, но оставил все поля в нем public, так как просто бомбануло при попытке написать в этот класс 24 геттера/сеттера и в том месте где я их заполняю
Ок, но другой вариант был не делать этот класс, а сделать геттеры в Company. Это позволяет получать отдельные цифры, не вычисляя все.

Еще один вариант - сделать только геттеры, без сеттеров, такого вида:

$info = new OrgInfo($company);
echo $info->getAvgSalary();

зачем тебе там сеттеры?

> Ну опять же кроме геттеров/сеттеров пустышек. На них тоже нужно выставлять тайпхинт по паравилу хорошего тона, даже если ты просто возвращаешь что-то без изменения/вычисления?
Желательно. Для полей ведь ты не указал типы значений, как понять, что там хранится? Плюс, это защищает от ошибок, например, попытки записать некорректное значение. Ты бы мог еще больше проверок сделать, например, запретить ставить неправильный ранг.

> Еще не понятно как быть в случае как с моей функцией getTopAnalyst() - либо она возвращает объект Analyst, либо null (по логике отсутствие объекта это как раз оно), и сюда ничего нельзя поставить на выходной тип, будут ошибки.

Есть тип ?Analyst со знаком вопроса. Гугли. Например https://wiki.php.net/rfc/nullable_types

> Но не стал склеивать методы demoteLeader() и promoteLeader() в один большой метод замены, просто заприватил их и оставил для лучшей читабельности. завел публичный swapLeader(), который просто вызывает их в нужной последовательности, логика в этом есть имхо.

Ну ок, тогда, получается, ты за возможность "обезглавить" департамент. Такое тоже возможно, почему бы и нет. Просто если оставить только swapLeader, то можно запретить делать департамент без босса или с несколькими боссами. А с promote/demote это возможно.

> В общем тут у меня проблема и не понимание как из этого слабого места выкрутиться. [про абстрактные методы]

Сделать абс. методы вроде getStartingSalary():float { return 500; } для каждого значения.

> И просто вынес это в отдельное поле да еще и выставляющееся в конструкторе. Не понял если честно что в этом плохого.
Ненужное дублирование данных, по моему. Можно подумать, что это какой-то отдельный список, не совпадающий с тем, что в компании. Ничего страшного в том, чтобы сделать getDepartments(), нету.

> Может быть стоит антикризис сделать вообще статическим классом?
Не думаю. Тогда ты должен будешь хранить компанию в статическом поле и нельзя работать с несколькими компаниями.

> А просто передавать ему объект, а он его будет возвращать в измененном виде аки обычная процедурная функция?
Вообще, можно.Но лучше бы обычный метод, а не статический. Так как в теории у "антикризисного менеджера" могут быть настройки (сколько процентов увольнять), зависимости, а для них нужны поля и $this.

Кстати, у нас еще есть задача про Гостиницу. Не хочешь отточить навыки ООП?
Ответы: >>1176480 >>1182534
ОП 2018/04/19 04:31:05  №1176480 5
Проверил в старом треде все оставшиеся задачи и посты, начиная с 30 марта:

- >>1176468 - задача про кредит и задача про вектор https://repl.it/repls/AcclaimedWhirlwindSoftwareengineer
- >>1176469 - почему в задаче про кредит не работает if ($a || $b || $c > $d) ?
- >>1176470 вопрос про ssh agent
- >>1176471 про просмотрщик картинок
- >>1176476 про ООП и https://github.com/mlmn/vector.loc/

Если вы писали в прошлом треде и вам не ответили - напомните о себе тут.
https://github.com/mlmn/vector.loc Аноним 2018/05/01 10:30:17  №1182534 6
1365085375083.jpg (321, 1680x1050)
1050x1680
1364843503614.jpg (1221, 1600x1200)
1200x1600
1403859616399.jpg (419, 2560x1600)
1600x2560
1355431579513.jpg (661, 985x739)
739x985
>>1176476
1. Вывод шаблонов в конструкторе и деструкторе убрал, сделал обычные методы.

2.
>А зачем нужно OrgInfo? Нельзя ли в организации сделать методы для вычислений этих чисел? Просто у меня ощущение, что этот класс заточен только под вывод таблицы, и тогда код getOrgInfo() желательно вынести из компании куда-нибудь, хотя бы в ту же OrgInfo. Или вообще ликвидировать этот метод.
Класс OrgInfo - выпилил, перенес вычисления общих и средних показателей по организации в отдельные методы. Теперь поясню почему я так держался за это с самого начала. Изначально очень удобно и просто было высчитывать всё это за 1 цикл, тупо прокручивая все департаменты в нем 1 раз, и складывая всю инфу в 1 выходной объект, поэтому в итоге это и выросло в такую вот шизу как отдельный класс для хранения этой информации на выходе этого метода. Мне и сейчас решение с вынесением всех вычислений в разные методы не кажется хорошим, просто потому что, во первых они почти все повторяют друг друга и у них отличается только переменная на входе, выходе и название соответственно, а во вторых всё это вместо одного компактного цикла использует каждый раз по циклу на метод и опять же перебирает снова и снова департаменты. В общем я сделал как рекомендовано, но остались сомнения с точки зрения оптимизации.

3. Тайп хинты расставил везде где нашел возможным.

4. try/catch выпилил, самому не нравилась идея оборачивать каждый код с exeptionon'ом внутри в эту конструкцию. А если я например чужой класс ипользую - мне нужно идти смотреть получается нет ли там внутри выброса исключений и на методы внутри которых есть рисовать try/catch каждый раз? Звучит тупо, так что только рад что это всё не нужно.

5.
>> public function promoteStuff(array $promotionList) {
>Я не думаю, что это надо делать в департаменте. Тут от департамента ничего не требуется и работников можно повышать напрямую.
Ок, сделал напрямую повышение, зачем лишние действия совершать.

6.
>По моему было бы гораздо удачнее сделать так:
>if ($employeeSelector->matches($employee)) ...
>Это позволило бы поместить логику отбора в класс-фильтра, где ей и место. Ну и еще есть такой вариант с коллбеком:
Перенес из департамента метод фильтрации сотрудников в сам класс EmployeeSelector, сделал там метод match который принимает на вход сотрудника и выдает булево значение - подходит сотрудник под текущие параметры или нет.
> new EmployeeSelector('Engineer', [1, 2, 3], [true, false]);
>лучше сделать возможность указать "не важно" для ранга. null, например, передать.
При этом сам способ фильтрации почти не изменился, разве что добавил возможность передавать в конструктор null как вариант "не важно"

7.
> return round($pageCost, 3);
>Не надо делать тут round, так как это нужно только для вывода в таблицу и должно быть там, где делается вывод, а не тут. Тут должен быть метод, который возвращает точное значение.
Ок, округление теперь во вьюхах

8.
> $vectorSecondAC = clone $vectorVanilla;
>В клонировании у тебя допущена ошибка. Этот метод не создает глубокую копию компании. он делает клон компании, но в него помещает ссылки на те же самые департаменты с теми же самыми работниками - они по умолчанию не клонируются, а просто копируются ссылки. Изучи магический метод __clone().
Переделал, теперь при клонировании организации должны клонироваться еще и объекты департаменты, а не просто ссылки на них, и при клонировании департамента соответственно объекты сотрудников

9.
> $this->organisation->setTitle("после антикризисных мер #1");
>Это неправильно, ради решения задачи про антикризисные меры добавлять в Компанию поле-комментарий. Это не нужно компании, это нужно только в антикризисных мерах, и это надо делать где-то в другом месте.
А мне это казалось клевым - мы изменили объект - мы отметил куда-то о том что он теперь другой и не нужно ниоткуда выцеплять это из других классов, комментарий идет сразу с объектом.
У нас ведь класс компании и служит не для каких-то там бизнесс-процессов компании, а для по сути отчетов, вот и это туда же.
Убрал, но как в другом месте это выводить или впиливать - не понял.

10.
>Сортировку лучше делать в одной функции, так:
>Также, есть еще такой трюк, вычисляем "вес" и сравниваем его:
Выбрал вариант с вычислением веса, так как он мне понятнее.

11.
>По view: в идеале, надо использовать htmlspecialchars при вставке текста в HTML. Иначе может быть уязвимость XS
Ок, обернул, только ведь это имеет смысл если мы выводим информацию, на которую как-то могут влиять сами юзеры, а так то кто в мою вьюху что подсунет. И вообще сделал обертку которая тупо сокращает длину названия функции, что бы было компактнее и удобнее её исользовать в перемешку с html

С простыми правками надеюсь что пока что всё, а не будет еще больше замечаний над которыми я просижу опять неделю.
Теперь опять к самому главному.
12.
>> abstract protected function setDefaults();
>Это не очень хорошо, так как непонятно, что эта функция должна делать. Это никак не описано и никак не проверяется. Мне непонятно, что в ней надо написать. По моему так лучше сделать функции getStartingSalary(), getStartingCoffee() и тд, с которыми все проще и понятнее.
>> В общем тут у меня проблема и не понимание как из этого слабого места выкрутиться. [про абстрактные методы]
>Сделать абс. методы вроде getStartingSalary():float { return 500; } для каждого значения.
Я вообще не понимаю что тут от меня нужно.

Теперь немного истории. Сначала я так и сделал, что в классе Employee просто сидели подобные абстрактные методы, и просто переопределялись для каждого нового класса сотрудника - это было удобно на этапе решения базовой задачи.

Но потом когда я начал решать антикризисные меры, там вы первых написано:
>Пришло время проверить, соответствует ли твой код принципам ООП? Гибок ли он и легко ли поддается изменениям?
В во вторых в одном методе там есть условие:
>Увеличить в целях стимуляции умственной деятельности базовую ставку аналитика с 800 до 1100 тугриков, а количество выпиваемого им кофе с 50 до 75 литров.
И тут становится понятно, что просто ретернить захардкоженные значения уже не работает. Ведь их придется менять, а что бы менять - их нужно где-то хранить (в полях и храним, а значит всё предельно удачно теперь встает на свои места и зменяемов с помощью опять же сеттеров и геттеры не просто хардкод а берут то, что в текущий момент в объекте установлено. Я опять же легко могу кому-угодно что угодно поменять, а как всё это провернуть с методами вроде этого:
>getStartingSalary():float { return 500; }
- я просто НЕ ПОНИМАЮ. Если у меня снова будут в сотруднике такие методы - то как я смогу вот эти вот 800 заменить на 1100 в коде?


13. Так же добавил свистелку в виде методов setTimePoint() и benchMark() - прошу оценить реализацию. Саму идею я уже где-то видел, но вот как обычно называется это под капотом и как раскидано по коду я не ковырял пока.

14.
>Кстати, у нас еще есть задача про Гостиницу. Не хочешь отточить навыки ООП?
Да хочу, лучше было сразу просто условие скинуть.


В итоге 2 главных проблемы так и не решил - OrgInfo ращбил на кучу копипастных методов, которые пусть и удобные, но само их существование мне не нравится и они дублируются и лишь загромождают код, и вообще ничего не понял как нужно сделать абстрактные методы в сотрудниках по типу: getStartingSalary():float { return 500; }

Ответы: >>1196649
https://github.com/mlmn/vector.loc Аноним 2018/05/25 10:48:41  №1196649 7
>>1182534

> try/catch выпилил, самому не нравилась идея оборачивать каждый код с exeptionon'ом внутри в эту конструкцию. А если я например чужой класс ипользую - мне нужно идти смотреть получается нет ли там внутри выброса исключений и на методы внутри которых есть рисовать try/catch каждый раз? Звучит тупо, так что только рад что это всё не нужно.

try/catch ты пишешь, только если тебе надо ловить исключение и как-то на него реагировать. Иначе не надо ничего писать. То, что функция выбрасывает исключение, не значит, что ты обязан его ловить. Это дело добровольное.

> Убрал, но как в другом месте это выводить или впиливать - не понял.
Просто прописать во вью либо сделать массив вида ['name' => ..., 'company' => ...] либо такой же объект.

> Ок, обернул, только ведь это имеет смысл если мы выводим информацию, на которую как-то могут влиять сами юзеры, а так то кто в мою вьюху что подсунет
Лучше сразу привыкать делать правильно. Плюс, если у тебя в названии компании будет <s> то без htmlspecialchars оно вставится как тег.

> >> abstract protected function setDefaults();
> Я вообще не понимаю что тут от меня нужно.
У тебя, чтобы объект Employee правильно работал, в нем надо задать зарплату, кофе и тд. Это можно сделать по-разному:

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

> И тут становится понятно, что просто ретернить захардкоженные значения уже не работает. Ведь их придется менять, а что бы менять - их нужно где-то хранить

Можно ввести 3 разных понятия:

- стартовая базовая зарплата - неизменна, возвращается абстрактными методами
- текущая базовая зарплата - хранится в поле, меняется. При создании объекта сюда копируется значение стартовой зарплаты.
- итоговая зарплата с учетом надбавок - нигде не хранится, вычисляется методом

Либо можно при создании объекта требовать указать стартовую зарплату в конструкторе.

В твоем коде никак не проверяется, что стартовая зарплата задана. Легко забыть это сделать при написании класса-наследника.

> Так же добавил свистелку в виде методов setTimePoint() и benchMark()
Почитай мануал по microtime - explode давно уже не нужен. Также, а зачем это нужно, если можно с помощью расширения xdebug записать трейс и в нем посмотреть, какие функции когда вызвались и сколько времени заняли? Погугли xdebug tracing.

> В итоге 2 главных проблемы так и не решил - OrgInfo ращбил на кучу копипастных методов, которые пусть и удобные, но само их существование мне не нравится и они дублируются и лишь загромождают код,
Тут наверно ничего не сделать, благо методы крошечные. Сложные классы могут содержать и по 1000, и по 2000 строк, так что ничего. Ну либо придется выносить это в обертку, которой мы передаем компанию и в которой есть методы для вычисления суммы и среднего.

Можно было еще как-то заморочиться с такой штукой, где один метод умеет считать сумму любого показателя:

public function getTotal($type) {
...
}

По коду:

Для методов, которые ничего не возвращают, можно писать тайп-хинт void.

> public function getDepartments(): array {
> if ($this->countDepartments() == 0) {
> throw new Exception('Пустая организация, нечего выводить.');
Я думаю, логичнее возвращать пустой массив. Разве это ошибка - запросить список департаментов, если их пока не добавили?

> $classMatch = (get_class($employee) == $this->getClass());
Может быть лучше instanceof, чтобы искались бы и наследники класса? По правилу подстановки Лисков наследник класса может использоваться вместо него.

> if ($classMatch and $rangMatch and $leaderMatch) {
> return true;
Можно писать return $classMatch && $rangMatch && $leaderMatch;

> if (in_array($employee, $fireList)) {
Тут наверно нужен еще true

> abstract protected function setDefaults();
Недостаток твоего решения тут в том, что этот метод не обязывает нас что-то делать. И не документирует, что он должен делать. Мы можем просто сделать пустой метод или забыть что-то в нем задать. Потому тут лучше было бы сделать 3 отдельных абстрактных метода вроде

abstract protected function getDefalutSalary(): float

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

> new EmployeeSelector('Engineer', null, null);
имя класса лучше писать как Engineer::class - тогда при опечатке будет выдана ошибка.

> $engineersList = $employeeSelector->filterEmployees($dep->getEmployees());
Тут еще можно было сделать вариант $dep->findEmployees($selector); но твой вариант тоже годится конечно.

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

$selector = new Selector;
$selector->setMinRang(2);
$selector->setMinExperience(2);
$selector->setSalaryRange(100, 500);

Также, есть еще интересный вариант, когда селектор выражается анонимной функцией, возвращающей true/false:

$list = $dep->findEmployees(function (Employee $e) {
return $e->getRank() == 2 && $e instanceof Engineer;
});

Если в PHP добавят стрелочные функции ( https://wiki.php.net/rfc/arrow_functions ), этот код будет еще короче:

$list = $dep->findEmployees(fn($e) => $e->getRank() == 2 && $e instanceof Engineer);

Более того, в объектах есть метод __invoke, который позволяет "вызывать" их как функцию: http://php.net/manual/ru/language.oop5.magic.php#object.invoke

Если твой Selector реализует этот метод, то его можно будет передавать в ту же самую функцию вместо анонимной функции.

В общем, сделано верно. Надеюсь, ты смог увидеть преимущества ООП в этой задаче, как ООП позволяет организовать большой и сложный код, разбив его на отдельные классы. Если хочется еще потренироваться с ООП, могу посоветовать задачу про Гостиницу: https://phpclub.tech/pr/res/1082507.html#1097078