Нормально ли обращаться к методам и свойствам классов через единый сервис?

Question

[Vladimir Krygin]

Добрый день!
Есть класс сервиса, через который идет обращение к свойствам и методам других классов (моделей). Реализовано примерно так:

namespace App\Services;

use App\Models\City;
use App\Models\Country;
use App\Models\Hotel;
use App\Models\Region;

class Service
{
    private Country $country;
    private City $city;
    private Hotel $hotel;
    private Region $region

  /**
     * @param $name
     * @return City|Country|Hotel|Region
     * @throws \Exception
     */
    public function __get($name)
    {
        switch ($name) {
            case 'country':
                return new Country();
            case 'city':
                return new City();
            case 'hotel':
                return new Hotel();
            case 'region':
                return new Region();
            default:
                throw new \Exception('Property ' . $name . ' is not exist.');
        }
    }
}

Обращения происходят так:

$service = new Service();
$some = $service->city->someMethod();
$field_name = $service->country->field_name;

Вопрос в том, насколько разумен такой подход и м.б. есть какие-то подводные камни?

Comments

[GavriKos]

Нормально ли каждый раз создавать новый инстанс? )
Вообще по сути это у вас фабрика.

[Vladimir Krygin]

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

[Adamos]

И что ваша IDE думает за подобные фокусы? Подозреваю, она говорит что-то вроде "придумал хрень, так и разбирайся в ней сам, я тебе не помощник".

[Vladimir Krygin]

Adamos, забыл приватные переменные добавить..
PHPStorm все нормально подсвечивает, подсказки выбрасывает и по Ctrl+клик переходит.

[GavriKos]

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

$service->city->setValue("100500");
$val = $service->city->getValue();

не даст вам в $val 100500? Может конечно пхп как то по другому работает, но скорее всего вот так

[Adamos]

GavriKos, с чего бы ему по-другому работать? Фактически, в этом коде просто (new City()) заменяется $service->city. Зачем-то. Никакой общей логики, требующей подобных извращений, ТС не приводит.

[Vladimir Krygin]

Adamos, смысл в том чтобы new'кать только то, что необходимо (чтобы через конструктор, не new'кать все разом). Да, согласен, инстансы классов внутри инстанса сервиса будут создаваться. (( может это обойти проверкой свойств в __get()?

[GavriKos]

Adamos, Крыгин Владимир, а, пардонте, да, обращение не через get же... Но в какой момент тогда инстансы создадутся и зачем этот гет отдельно наружу выплевывает новый инстанс? Или это уже пхп-шная магия? (вряд ли)

[Mikhail Osher]

Похоже на Service Locator.

[Vladimir Krygin]

GavriKos, да, магия. При обращении к private свойству вызывается метод __get().

[Adamos]

GavriKos, вы нам показали кусок кода ни о чем и предлагаете погадать на гуще?
Попробую пованговать: у вас должны быть единственные экземпляры-синглтоны указанных классов, просто вместо этой логики сделано через задницу - с созданием экземпляра на каждый запрос.

[GavriKos]

Крыгин Владимир, тогда прошу мои комменты считать невалидными )

[Adamos]

GavriKos, извините, промазал ;)

[Vladimir Krygin]

Mikhail Osher, спасибо за подсказку. Буду переделывать согласно паттерна.

[Vladimir Krygin]

Adamos, спасибо, что указали на проблему. Но, чесслово, не стоит так близко к сердцу принимать вопросы. ;)

[Mikhail Osher]

Крыгин Владимир, как раз дело в том, что Service Locator - антипаттерн. Его надо избегать.

[Vladimir Krygin]

Mikhail Osher, почитал, считают антипаттерном, но не всегда, и как я понял смотря для каких целей. синглтоном пользуются же. в моей ситуации будет норм.

[Mikhail Osher]

Крыгин Владимир, синглтоном пользуются в исключительных ситуациях

Answer 1

[Mercury13]

С моей колокольни статических языков такая конструкция — избыточная сложность на пустом месте.
Но есть места, где такая сложность ещё и чем-то оправдана. Например, Qt — содержимое ячейки таблицы…

QVariant SomeModel::data(
    const QModelIndex &index, int role = Qt::DisplayRole) const override {}

Для чего оно такое в Qt…
• в самом начале функции может вычисляться адрес в памяти, где все эти данные находятся, а за ним — длинный switch/case «в зависимости от роли, возьми то-то»;
• возможно возвращение пустого QVariant, когда надо сказать: «действуй как обычно»;
• наконец, возможны сложные операции с данными, вроде преобразования в отображаемую форму и сортировки, которые не зависят от специфики модели данных.

Так что ответ. Смотрите по месту: что вы хотите этой конструкцией сказать и почему более простые не оправданы.

Comments

[Vladimir Krygin]

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

[Mercury13]

Крыгин Владимир, А почему не getCountry() и getCity()?

Answer 2

[Антон Шаманов]

поздравляю, ты открыл для себя шаблон проектирования фасад

Comments

[kafkiansky]

Больше похоже на популярный шаблон "говнокод"

[Антон Шаманов]

kafkiansky, одно другому не мешает.
на фоне того что я видывал, это еще ничего, вот поковыряешься в легаси 10летней давно где не то что тестов нет - там моделей нет и контроллеры по 2-3к строк..

[Кирилл Несмеянов]

kafkiansky, и почему это говнокод? Вот вполне себе юзабельный пример из того, с чем я сталкивался недавно: https://github.com/KnpLabs/php-github-api/blob/mas...

[kafkiansky]

Кирилл Несмеянов, потому что в случае, когда надо будет добавить еще обработчики, надо будет лезть в этот код и добавлять кейсов. Вместо того, чтобы передать типизированный список и пройтись по нему форичем, используя какой-нибудь support($arg), как часто делают. switch..case ок, когда список точно не будет меняться: скажем, если мы конвертируем картинки, можно обойтись свитчом, потому что новые расширения для картинок появляются не каждый день, а всякие штуки для апи или бизнес-объекты могут как появляться, так и исчезать постоянно.

[Кирилл Несмеянов]

kafkiansky, а, ну это да, он нарушает open-close.

[Антон Шаманов]

Кирилл Несмеянов, это говнокод не из-за каких-то свичей, это говнокод потому что такие сокращения не делают код короче лучше и даже оптимальнее. Хочешь писать быстро - используй ide, они для этого и создаются, но ide плохо дружат с такими решениями и аннотации тут не помогут. Github/Client - говнокод потому, что он нарушает SOLID принципы. Пора понять что "популярный" - не всегда синоним "правильный/оптимальный".

[Кирилл Несмеянов]

Антон Шаманов, IDE как раз отлично дружат с такими решениями, если что. Там докблоки для этого дела прописаны все

[Антон Шаманов]

Кирилл Несмеянов, какая именна ide у тебя дружит с этим кодом?

[Кирилл Несмеянов]

Антон Шаманов, шторм конечно. Да, боюсь, что вообще любая адекватная в современном мире умеет анализировать докблоки.

[Vladimir Krygin]

Антон Шаманов, не ссоритесь ;) phpstorm нормально работает и с более извращённым конструкциями. Если Кирилл Несмеянов увидел тут говнокод, то это его проблемы. Не все нарушающее SOLID плохо, зависит же от ситуации.

[Кирилл Несмеянов]

Vladimir Krygin, а я то тут причём? о_0 я наоборот говорю, что вполне норм. А то что он нарушает open-close - это мелочи, которые в данной ситуации несущественны.

[Антон Шаманов]

Кирилл Несмеянов Vladimir Krygin диалог это нормально, та версия phpstorm что стоит у меня отобразит в автоподстановке методы всех перечисленных классов, а не конкретно того что возвращается. и если в случае с Github/Client::api() от такой автоподстановки мало толку.
@jumpordie

Не все нарушающее SOLID плохо, зависит же от ситуации.

я не зря подчеркнул S - не припоминаю когда нарушение принципа единой ответственности было бы на пользу.

[Кирилл Несмеянов]

> та версия phpstorm что стоит у меня отобразит в автоподстановке методы всех перечисленных классов, а не конкретно того что возвращается.

Эм... я тут по истории смотрю. Ну в общем они этот функционал добавили в PhpStorm 4.0 (судя по issue): https://youtrack.jetbrains.com/issue/WI-10028 Вы точно уверены, что у вас шторм 2010го года выпуска? =)

> и если в случае с Github/Client::api() от такой автоподстановки мало толку.

Мне казалось что тред о том насколько корректно скрывать объекты за геттерами. Я привёл в пример аналог другой библиотеки с идентичным подходом. Т.е. мы обсуждаем (надеюсь) конкретно метод "__get", а не "api".

У меня, к слову, есть похожая библиотека в репозитории, которая была написана 100500 лет назад и которая тоже построена по этому принципу: https://github.com/SerafimArts/gitter-api/blob/mas...

[Антон Шаманов]

Кирилл Несмеянов,

что у вас шторм 2010го года выпуска?

я хз есть ли разница между моей open source edition и платной, но моя версия явно использует статичный анализ, а не динамический т.ч. либо плагин какой-то либо кто-то тут меня дурит (:

мы обсуждаем (надеюсь) конкретно метод "__get", а не "api".

сложно обсуждать то чего нет

[Кирилл Несмеянов]

Антон Шаманов, предлагаю ещё раз с самого начала треда, т.к. есть подозрения, что мы обсуждаем совершенно разные вещи.

Проблема: Норм ли в __get фигачить switch/case и возвращать объекты. Я написал что "норм" и привёл примеры либ (knp и свою), где так вполне делается и пока не мешало.

Вы же на скрине совершенно иной кейс используете, где всего этого нет. Как следствие и автокомплита нормального.


https://habrastorage.org/webt/p1/eu/zq/p1euzqnt3aw...

[Кирилл Несмеянов]

Вы же на скрине совершенно иной кейс используете, где всего этого нет. Как следствие и автокомплита нормального.

Антон Шаманов, такие задачи уже не через докблоки делаются, а через метадату. На выходе получаем что-то такое:

spoiler

https://habrastorage.org/webt/zp/dd/1p/zpdd1phq6e5...


https://habrastorage.org/webt/ua/pr/yb/uapryb0wlue...

[Антон Шаманов]

Кирилл Несмеянов, это одни и те же яйца - вид сбоку, я уже писал про статический и динамический анализ - динамический это когда делаешь трассировку кода и ide тебе выдает конкретно сейчас в данной переменной. У KnpLabs все не так плохо из-за единого интерфейса получаемых объектов, но в любом случае фабрике там не место. у по поводу меты - как вообще ты смог это запустить? ключевые слова...

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