«phpClub» — архив тем ("тредов"), посвящённых изучению PHP и веб-технологий.
Аноним 2018/12/08 22:27:38  №1308331 1
>>1305368 (OP)

https://ideone.com/YZhoeC

ООП-Будильник 2.0 с учетом замечаний из предыдущего треда, надеюсь ничего не упустил. Спасибо за подробные замечания, всё очень доступно и понятно!

https://ideone.com/Kj2GBG
Версия с тестами, выглядит не так стильно но работает. Ну эмм, я пошел делать вектор.
Ответы: >>1311510 >>1313228 >>1314975
Аноним 2018/12/15 12:28:01  №1311510 2
Аноним 2018/12/18 17:01:23  №1313228 3
Ответы: >>1314975
Аноним 2018/12/22 13:01:05  №1314975 4
>>1313228
>>1308331

Комментариев, мягко, говоря маловато. Непросто будет другому разработчику понять, как использовать код. Хотя бы комментарий перед классом стоило добавить (а в идеале перед всеми неочевидными публичными методами).

> $this->time = $hour.':'.$minute;
А не выгоднее ли хранить время как часы и минуты отдельно, чтобы не мучаться с преобразованиями?

> $hour = 0..$hour;
А зачем тут 2 точки? Это получается 0. (0 без дробной части) . $hour.

> $minute > 60
А 60 минут - это допустимо?

> $datesAndTimes[] = $currentTime->modify($ordinal.' '.$spelling[$dayOfWeek].' '.$this->time);
Интересное решение.

> public function setAlarm(int $id, Alarm $alarm): void
> public function getAlarm(int $id): ?Alarm
А зачем этот id? Для тестов? Мне кажется, проще вообще без него. Или ты не уверен, что когда ты меняешь свойства тревоги, они поменяются внутри AlarmClock ? А это уже к тебе вопрос: поменяются или нет?

> if ($object == $alarm) {

Изучи, в чем отличие == и === для объектов в PHP мануале: http://php.net/manual/ru/language.oop5.object-comparison.php

> public function findNearestAlarm(DateTimeImmutable $currentTime): Alarm
Возможно, было бы лучше использовать DateTimeInterface, чтобы можно было передавать и DateTime, хотя тогда придется беспокоиться, что ты не поменяешь его содержимое.

> $alarms[] = [$alarmTime, $alarm];
> sort($alarms);

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

> sort($datesAndTimes);
> return $datesAndTimes[0];

Здесь есть проблема: если ни один день недели не выбран, то массив будет пуст и произойдет ошибка (для проверки, что ошибки не будет, можно написать тест).

> sort($alarms);
> return $alarms[0][1];
То же самое.

> private function isTriggeredAlarm(Alarm $alarm, DateTimeImmutable $currentTime): bool
Мне кажется, тут было бы быстрее сравнить время, день недели и признак активности. Хотя использованный подход тоже годится.

> $currentTime = new DateTimeImmutable('13:00');
Здесь в тесте появляется зависимость от текущего дня. Лучше бы жестко прописать дату, чтобы убрать случайности.

> assert(get_class($alarmTime) == DateTimeImmutable::class);
Лучше было бы писать $alarmTime instanceof DateTimeImmutable - ведь мы не обязаны вернуть строго этот класс, а допустимо вернуть его наследника в соотв. с принципом Лисков (LSP).

> assert($alarmClock->getAlarm(0) === null);
Вот тут вместо поиска по id (а откда ты знаешь, какой у тревоги id? Это нигде не документировано) было бы лучше сделать метод вроде getAllAlarms() или hasAlarm() и проверять через них. А с твоим подходом - появляется необходимость в id, и тест становится хрупким, зависящим от деталей реализации класса: если завтра мы начнем нумеровать тревоги с 1, то тест сломается. А это затраты времени и денег на переписывание.

> assert($nearestAlarm === $alarm || $nearestAlarm === $alarm2);

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

> $alarm2->setType(Alarm::TYPE_NON_REPEATING);
> $alarmClock->setAlarm(0, $alarm2);
Вообще, вызывать setAlarm не требуется. Ты объект заменяешь тем же самым объектом.

В общем, сделано хорошо.

Ну и на будущее, сейчас ты писал тесты руками, но вообще есть готовые фреймворки для написания тестов, где много готовых полезных методов, где есть вывод отчетов о выполнении тестов. Самый известный - это PhpUnit, это PHP-версия семейства фреймворков xUnit, которые есть почти для любого языка (версия для Явы, например, называется JUnit). Это семейство началось с фреймворка для Smalltalk SUnit, описанного Кентом Беком в статье 1989 года ( http://swing.fit.cvut.cz/projects/stx/doc/online/english/tools/misc/testfram.htm - там есть полный код фреймворка). Почти 30 лет прошло!

Чтобы пользоваться PhpUnit, нужно его установить через композер, но если ты пока с ним не знаком, то его можно просто скачать в виде .phar-файла.
Ответы: >>1315215 >>1316084
Аноним 2018/12/22 18:47:32  №1315215 5
>>1314975
> Возможно, было бы лучше использовать DateTimeInterface, чтобы можно было передавать и DateTime

Я не тот, кому адресован ответ, просто для справки отвечу - DateTimeInterface лучше не использовать. Это бесполезный интерфейс, так как если функция работает с аргументами типа \DateTime, на которых вызываются методы modify или setTime, то при передаче в функцию аргументов типа \DateTimeImmutable функция вернёт другие результаты, верно и обратное.
Например во Flow даже есть специальные конструкции, позволяющие пометить аргументы, для которых запрещено мутирование: https://flow.org/en/docs/types/interfaces/#toc-interface-property-variance-read-only-and-write-only

Ещё есть RFC от разработчика доктрины, предлагающее выпилить DateTimeInterface: https://wiki.php.net/rfc/drop-datetimeinterface
Похоже, что этот интерфейс вводился для поддержки перезрузки операторов вроде >, < для объектов дат.

Иммутабельные объекты-значения вроде денег и времени исключают огромное количество трудноуловимых багов. Кстати, в мире JS есть библиотека moment, в которой даты не иммутабельны и разработчики писали пост с извинениями за то, что сразу не сделали нормально. Сейчас уже слишком поздно что-то менять из-за обратной совместимости, поэтому приходится постоянно клонировать даты.
Ответы: >>1315228
Аноним 2018/12/22 19:06:05  №1315228 6
>>1315215

Вообще, да, справедливое замечание. Не стоит тогда менять DTImmutable на DTInterface.

Я ловил себя на мысли, что эти классы не очень совместимы, но как-то глубоко не думал.

Действительно, интерфейс определяет не только названия методов, но и определенные требования к ним. И неправильно, когда одна реализация modify создает новую сущность, а другая - модифицирует существующую. Трудно написать интерфейс, который корректно работает с обоими типами.
Ответы: >>1315231
Аноним 2018/12/22 19:07:04  №1315231 7
>>1315228

* трудно написать код, который корректно работает с обоими типами
Аноним 2018/12/24 18:22:13  №1316084 8
>>1314975
Спасибо за замечания!

>Комментариев, мягко, говоря маловато
Понял, сделаю

>А зачем этот id?
Переделаю

>Интересное решение.
> $currentTime->modify($ordinal.' '.$spelling[$dayOfWeek].' '.$this->time);

Что-то не так? Есть вариант с $time-add(DateInterval::createFromDateString('параметры')); , но он выглядит слишком массивно и по-моему имеет какой-то подвох.

>Здесь есть проблема: если ни один день недели не выбран, то массив будет пуст и произойдет ошибка

Но это невозможно, массив с днями недели не может быть пустым.

У меня возник вопрос насчет функции setAlarm. Как она в нынешнем виде (замена объекта на объект) может пригодиться в реальном проекте? Ведь можно завести метод getAlarms():array и настраивать нужный нам объект выбрав его из массива. Это нарушит принцип solid?
Ответы: >>1319269
Аноним 2018/12/31 20:08:58  №1319269 9
>>1318327

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

В PHP огромное число, тысячи функций. Без разделения на расширения будет тяжело ориентироваться.

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

>>1317102

У меня буквы почему-то маленькие. Трудно читать в таком виде.

>>1316313

> Составил такую регулярку
Возможно, проблема в том что точка соответствует любому символу кроме перевода строки. Почитай официальный мануал по рег. выражениям http://php.net/manual/ru/pcre.pattern.php если не знал.

>>1316164

Сайтмап удобно генерировать методами DOM так как это обеспечивает его корректность и экранирует все нужные символы. Гугли DOMDocument. Не надо тут делать велосипеды и писать уродливые циклы с конкатенацией строк.

>>1316084

Я написал "интересное решение" без иронии, потому исправлять там ничего не надо.

>>Здесь есть проблема: если ни один день недели не выбран, то массив будет пуст и произойдет ошибка
> Но это невозможно, массив с днями недели не может быть пустым.

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

> У меня возник вопрос насчет функции setAlarm. Как она в нынешнем виде (замена объекта на объект) может пригодиться в реальном проекте? Ведь можно завести метод getAlarms():array и настраивать нужный нам объект выбрав его из массива. Это нарушит принцип solid?

Она никак не может пригодиться и довольно бессмысленна, и ты скорее всего ее написал, так как не выучил, как копируются указатели на объекты. Если ты пишешь:

$a = new Alarm();
$b = $a;
$a->setEnabled(true);

То тут $a и $b указывают на один объект и писать $b->setEnabled() или дополнительное $b = $a не требуется. А ты зачем-то это пишешь.

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