«phpClub» — архив тем ("тредов"), посвящённых изучению PHP и веб-технологий.
Аноним 2020/08/06 14:42:27  №1770074 1
>>1731888 (OP)
ОП, проверь Вектор - https://ideone.com/06qnUR сделал с антикризисными мерами.

Пробовал создавать новые объекты vector через сlone, не получалось (данные были одинаковы во всех остальных после применения антикризисных мер), пришел к сериализации.
Ответы: >>1786072 >>1792962
Аноним 2020/08/30 17:17:25  №1792962 2
>>1770074 →

> Пробовал создавать новые объекты vector через сlone, не получалось

Если ты клонируешь объект, содержащий другие объекты (например, клонируешь Департамент, в котором есть Работники), то основной объект клонируется, а вложенные - нет, просто в объект-клон копируются ссылки на них. Надо использовать магический метод __clone и в нем вручную клонировать вложенные объекты.

Антикризисные меры логичнее сделать в отдельном объекте ("антикризисный комитет"). Так как у каждого класса должна быть своя зона ответственности, а в твоем примере Департамент отвечает и за управление списком работников, и за антикризисные меры. Дальше добавится еще задача, ты ее снова засунешь в Департамент, и получится класс-монстр, который занимается всем на свете.

> dismissEmployee(int $id)

Неудачно сделан аргумент - id - откуда мы его должны брать, он ведь не прописан в Работнике? Логичнее было бы сюда передавать объект-работника и пусть класс сам разбирается, как его найти.

Увольнение инженеров лучше было бы сделать так:

- выбрать всех инженеров (метод в департаменте)
- отсортировать по рангу (usort)
- отрезать первые 40% (array_slice)
- уволить по получившемуся списку

То есть тут можно использовать готовые функции работы с массивами вместо сложных циклов с кучей if внутри.

> $job = get_class($search->getJob());
> $search->setJob(new $job(), $maxRank, true);

Тут стоило сделать метод changeRank() в Employee. Так как этот класс отвечает за хранение и обновление ранга, и код логичнее всего поместить в него. И $e->changeRank(...) проще читать, чем это.

Неудачно сделан учет ранга. Логично иметь объект Job с базовыми (неизменными) ставками, и отдельно поле rank, и при расчете зарплаты учитывать ранг. А ты вместо этого меняешь сами базовые ставки в объекте Job. Если вызвать метод updateJob() несколько раз, то базовая ставка будет многократно увеличена:

$emp->updateJob(3);
$emp->updateJob(3);
$emp->updateJob(3);

То есть этот метод работает некорректно.
Ответы: >>1793026 >>1794773
Аноним 2020/08/30 17:58:12  №1793026 3
>>1792962
Спасибо. Сделаю рефакторинг и отправлю еще раз =)
Аноним 2020/09/01 11:19:51  №1794773 4
>>1792962
- оставил сериализацию
- сделал увольнение сотрудников по схожести объекта (Если один равен другому - удалить)
- прилепил ко всему встроенные функции, избавился от кучи условий
- зарплата считается на основе данных в классе профессии. (базовая ставка изменяется только для антикризисных мер)

Теперь правда отличаются прошлые данные (с ideone) и текущие.
Прочитал измененный код, вроде все должно быть хорошо =).

https://github.com/Back1ng/vector
Ответы: >>1807633
https://github.com/Back1ng/vector/ Аноним 2020/09/15 19:11:12  №1807633 5
>>1794773

Сериализация, это, конечно, костыльно, лучше бы ты разобрался с оператором clone и магическим методом __clone. Сериализация тебя может подвести, если в графе объектов есть циклические ссылки (объект A хранит в себе B, а B хранит в себе A, например, Работник хранит в себе ссылку на Департамент, а тот - на Работника).

В комитете, мне кажется, логичнее было бы передавать Компанию в метод setAntiCrisisMeasuresFirst($vector), а не в конструктор. Тогда один Комитет мог бы оптимизировать хоть несколько Компаний подряд.

> usort($engineers, function($a, $b) {
> if ($a->isLeader() && $b->isLeader()) return 0;
> if ($a->isLeader()) return 1;
> return $a->getRank() <=> $b->getRank();
> });

Тут не хватает if ($b->isLeader()) { return -1 }. Или, что лучше, написать так: if ($a->isLeader() != $b->isLeader()) { return (int)$a->isLeader() <=> (int)$b->isLeader(); }.

> array_map(function ($employee) use ($department) {
Тут, наверно, читабельнее было бы использовать foreach, ведь array_map предназначен для преобразования массива в другой, и это только сбивает с толку. Не стоит использовать array_map как замену для foreach, стоит использовать его там, где мы преобразуем один массив в другой.

Для замены босса было бы логично сделать в Департаменте метод replaceBoss($newBoss), а не руками переключать признак босса в цикле.

inverseLeader() хуже читается, чем makeLeader()/demoteLeader(), в последних явно видно, какое значение мы ставим. Это неудачная функция, и трудно представить ситуацию, где нам нужно именно инвертировать признак босса, а не установить конкретное значение да/нет.

> if (!($leader instanceof Analyst)) {
Тут по моему ошибка: $leader->getJob(). На такой случай можно бы сделать метод $emp->isJob(Analyst::class).

> * @param Department $department
Тип аргумента уже указан в тайп-хинте функции, незачем эту информацию дублировать, если в комментарии не содержится ничего нового.

> public function getAverageExpenses()
Тут можно было не дублировать код из функции getExpenses() , а вызвать ее.

> @return Employee|false
Лучше возвращать null, тогда можно указать тайп-хинт ?Employee - Employee или null. (кстати, в новом PHP будут union types - можно писать func(): Employee | false ).

> public function setRate(int $rate) : self
> $this->rate = $rate;
Тут у тебя ошибка - ты обращаешься к несуществующему в классе Job свойству. Нельзя обращаться к полям и методам, которые появятся только в наследниках. Так как когда кто-то будет наследовать твой класс, он может забыть добавить в него поля и методы и будет ошибка.

Правильнее было бы сделать в Job абстрактные методы getBaseRate(), getBaseCoffee() и тд, а в наследниках их переопределять.

На будущее, есть статические анализаторы кода вроде phpstan ( https://phpstan.org/ ), они покажут такие ошибки (и некоторые IDE тоже могут их находить). phpstan освоить несложно, достаточно скачать PHAR-файл и запустить из командной строки. Можешь попробовать освоить его. Поставь уровень побольше (опция -l). Также, есть статический анализатор psalm.

В остальном, не так и плохо. Если захочется еще повозиться с ООП, можешь найти задачу про ООП-гостиницу. Думаю, навыки работы с ООП тебе помогут. когда будешь разбираться с фреймворками, хотя там конечно абстракции будут посложнее.