4.4. Присвоения в условных операциях

[peter-gribanov]

4.4. Присвоения в условных операциях

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

if (
    ($token = $this->token_storage->getToken()) &&
    ($user = $token->getUser()) &&
    $user instanceof User
) {
    // do something
}

VS

$token = $this->token_storage->getToken();
if ($token instanceof TokenInterface) {
    $user = $token->getUser();
    if ($user instanceof User) {
        // do something
    } else {
        // мы должны писать return или else, что бы не менялось поведение программы
    }
}

Пример посложнее

if (
    ($request = $this->request_stack->getMasterRequest()) &&
    ($session = $request->getSession()) &&
    ($token = $this->token_storage->getToken()) &&
    ($user = $token->getUser()) &&
    $user instanceof User &&
    ($confirm_token = $session->get('confirm_token')) &&
    $user->getConfirmToken() === $confirm_token
) {
    // do something
}

VS

$request = $this->request_stack->getMasterRequest();
$token = $this->token_storage->getToken()
if ($request instanceof Request && $token instanceof TokenInterface) {
    $session = $request->getSession();
    $user = $token->getUser();
    if (
        $session instanceof SessionInterface &&
        $user instanceof User &&
        $session->has('confirm_token')
    ) {
        $confirm_token = $session->get('confirm_token');
        if ($confirm_token && $user->getConfirmToken() === $confirm_token) {
            // do something
        } else {
            // обрабатываем исключительную ситуацию
        }
    } else {
        // обрабатываем исключительную ситуацию
    }
}

[index0h]

Жуть какая.

$request = $this->request_stack->getMasterRequest();

if (!($request instanceof Request)) {
    // ...
}

$token = $this->token_storage->getToken();

if (!($token instanceof TokenInterface)) {
    // ...
}

$session = $request->getSession();

if (!($session instanceof SessionInterface)) {
    // throw new ...
}

if (!$session->has('confirm_token')) {
    // throw new ...
}

$user = $token->getUser();

if (!($user instanceof User)) {
    // throw new ...
}

if ($user->getConfirmToken() !== $session->has('confirm_token')) {
    // throw new ...
}

// ...

Если всюду возврат по nullable интерфейсам то лучше так:

$request = $this->request_stack->getMasterRequest();

if (is_null($request)) {
    // ...
}

$token = $this->token_storage->getToken();

if (is_null($token)) {
    // ...
}

$session = $request->getSession();

if (is_null($token)) {
    // throw new ...
}

if (!$session->has('confirm_token')) {
    // throw new ...
}

$user = $token->getUser();

if (is_null($user)) {
    // throw new ...
}

if ($user->getConfirmToken() !== $session->has('confirm_token')) {
    // throw new ...
}

// ...

[peter-gribanov]

Не все nullable. И на null правильно проверять так:

if ($token === null) {
    // ...
}

Ваш вариант выглядит ещё ужасней и приведет к куче дублируемого кода

[index0h]

И на null правильно проверять так

почему?

Ваш вариант выглядит ещё ужасней и приведет к куче дублируемого кода

На счет ужасней - очень спорно. На каждом шаге я знаю, что у меня есть, и что валидно, получив исключение я могу добавить туда деталей на каждом шаге, coverage отлично отрисует, что я покрыл, а что нет. Ваш же пример - это сплошной комок (причем не тождественный вами же отрефакторенному примеру). Допустим: ($user = $token->getUser()) && вернул null, по идее это ситуация исключительная, но что вы бросите и запишете в лог?

На счет дублирования - в каких местах? Если вы про $request и $token - да не вопрос, можно проверки объединить. Если вы про исключения - вот ту не согласен, в каждом случае у вас исключение должно отличаться.

[peter-gribanov]

почему?

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

Допустим: ($user = $token->getUser()) && вернул null, по идее это ситуация исключительная, но что вы бросите и запишете в лог?

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

$request = $this->request_stack->getMasterRequest();

if (!($request instanceof Request)) {
    return;
}

$token = $this->token_storage->getToken();

if (!($token instanceof TokenInterface)) {
    return;
}

$session = $request->getSession();

if (!($session instanceof SessionInterface)) {
    return;
}

if (!$session->has('confirm_token')) {
    return;
}

$user = $token->getUser();

if (!($user instanceof User)) {
    return;
}

if ($user->getConfirmToken() !== $session->has('confirm_token')) {
    return;
}

// ...

Бесконечная череда return выглядит не очень и может привести к ошибкам.

[index0h]

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

Ок, согласен

Бесконечная череда return выглядит не очень и может привести к ошибкам.

На счет не очень выглядит - очень субъективно. На счет ошибок - не согласен, аргументируйте.

[peter-gribanov]

На счет ошибок - не согласен, аргументируйте.

Больше количество однотипного кода замыливает глаз. Можно легко пропустить где-то return или в процессе рефакторинга случайно его удалить и т.д. Много точек выхода из функции. В моем же примере точек выхода всего 2 и как бы код не менялся их число не изменится. Я согласен с тем, что код выглядит нагроможденным, но он более безопасный и он ясно даёт понять, что он будет работать только если все истино. С вашим кодом это не очевидно.

[index0h]

Можно легко пропустить где-то return или в процессе рефакторинга случайно его удалить и т.д.

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

Много точек выхода из функции.

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

В моем же примере точек выхода всего 2 и как бы код не менялся их число не изменится. согласен с тем, что код выглядит нагроможденным, но он более безопасный

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

    ($request = $this->request_stack->getMasterRequest()) &&
    ($session = $request->getSession()) &&
    ($token = $this->token_storage->getToken()) &&

В вашем же примере вы не проверяете интерфейсы $request, $session, $token.

[peter-gribanov]

В вашем же примере вы не проверяете интерфейсы $request, $session, $token.

Да, не проверяю потому, что интерфейс мне говорит, что возвращаемое значение Request|null, TokenInterface|null и SessionInterface|null соответственно. Для User это не так. Соответственно мне достаточно проверить выражение на истину и не обязательно проверять тип.

[index0h]

Да, не проверяю потому, что интерфейс мне говорит

Тогда наши примеры соответствуют разным условиям))

$request = $this->request_stack->getMasterRequest();
$token = $this->token_storage->getToken();

if ($request === null || $token === null) {
    return;
}

$session = $request->getSession();
$user = $token->getUser();

if ($session === null || $user === null || !$session->has('confirm_token')) {
    return;
}

if ($user->getConfirmToken() !== $session->get('confirm_token')) {
    return;
}

// ...

Либо так

$request = $this->request_stack->getMasterRequest();
$token = $this->token_storage->getToken();

if ($request === null || $token === null) {
    return;
}

$session = $request->getSession();
$user = $token->getUser();

if ($session === null || $user === null || $user->getConfirmToken() !== $session->get('confirm_token')) {
    return;
}

[index0h]

@peter-gribanov Я тут провел небольшой бенчмарк по is_null vs === null. Почему-то вызов is_null работает быстрее, хотя по op-кодах вариант с оператором сравнения меньше.

https://3v4l.org/WQk7Q Локально проверял на большем количестве итераций, несколько раз, и тенденция сохранялась. Почему так я не понимаю, у вас есть соображения на этот счет?

[index0h]

@peter-gribanov Хотя... кажется я догнал https://3v4l.org/cq2aN/vld#output

В случае $value === null сравнивается И значение И тип. В случае is_null($value) сравнивается только тип.

Если я наглупил в бенчмарке - укажите пожалуйста.

[peter-gribanov]

Судя по всему, в последних версиях PHP что-то хорошо оптимизировали. Раньше вызов функции был всегда медленнее:

• https://www.php.net/manual/ru/function.is-null.php#84161
• https://stackoverflow.com/a/8229059

И в вашем примере https://3v4l.org/WQk7Q для PHP версии 7.2.25 и ниже вызов функции медленнее чем операция сравнения.

В таком случае, пожалуй стоит использовать функцию is_null() в последних версиях PHP.

[peter-gribanov]

Что интересно, если добавить namespace, то is_null() будет выполнятся медленнее, а операция сравнения будет выполнятся за то же время. А по скольку мы редко пишем код без namespace, то все же выгодней операция сравнения.

https://3v4l.org/W0GRc https://3v4l.org/AvB7A/vld#output

[index0h]

Добавление глобального неймспейса к is_null убирает поиск по текущему неймспейсу.

https://3v4l.org/BcAc0/vld#output https://3v4l.org/99I4L

Как думаете, есть ли смысл добавить пункт по обязательному указанию глобального неймспейса?

[peter-gribanov]

Как думаете, есть ли смысл добавить пункт по обязательному указанию глобального неймспейса?

Я бы не стал. Указывать для каждой функции глобальный неймспейс неудобно.

[EvgeniiR]

@peter-gribanov Тут не присваивание в сравнениях разрешать нужно, а рефакторить код так чтобы этих присваиваний и этих сравненией не было. Это жесть какая-то в обоих случаях.

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

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

[peter-gribanov]

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

@EvgeniiR миделвари в данном случае это размазывание кода по проекту.

[EvgeniiR]

@peter-gribanov

миделвари в данном случае это размазывание кода по проекту.

Давно пора перестать писать весь код в одном файле.

[peter-gribanov]

@EvgeniiR Извините. Случайно отправил недописанное сообщение. Естественно писать все в одном файле плохо, так же как и размазывать 3 строчки кода на 5 классов. Во всем надо знать меру.

Говоря о миделвари вы наверное имели в виду его в контексте CQRS. Миделвари в контексте CQRS хорошая вещь, но она добавляет немного магии и это не всегда хорошо. Это примерно как Event-driven architecture. Хорошо, но в меру.

[EvgeniiR]

@peter-gribanov

Естественно писать все в одном файле плохо, так же как и размазывать 3 строчки кода на 5 классов. Во всем надо знать меру.

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

Говоря о миделвари вы наверное имели в виду его в контексте CQRS. Миделвари в контексте CQRS хорошая вещь, но она добавляет немного магии и это не всегда хорошо.

Никакой CQRS тут не причём, и никакой магии нет. Гляньте на досуге https://www.youtube.com/watch?v=v1I57-_Rsv0 , чтобы не писать 2007-style.

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

[peter-gribanov]

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

Как я уже говорил предыдущему комментатору, тут нет ветвления. Тут тот кейс, когда нас интересует только TRUE ветвь. Все FALSE ответвления просто игнорируются. А если это "полотно" еще и в Event handler, а не в контроллере, то распылятся на миделвари вообще может быть не резонно. И если в do something у нас просто дергается сервис и никакой бизнес логики тут нет, а по сути это роутер или пример реализации одного из слоев миделвари.

Никакой CQRS тут не причём, и никакой магии нет. Гляньте на досуге https://www.youtube.com/watch?v=v1I57-_Rsv0 , чтобы не писать 2007-style.

Ну это не ново. Миделвари на базе PSR-15 сильно ограниченные. Лучше подойдут классические миделвари коих море https://github.com/gpslab/middleware Рекомендую еще глянуть АОП из 2001.

[EvgeniiR]

Как я уже говорил предыдущему комментатору, тут нет ветвления. Тут тот кейс, когда нас интересует только TRUE ветвь. Все FALSE ответвления просто игнорируются. А если это "полотно" еще и в Event handler, а не в контроллере, то распылятся на миделвари вообще может быть не резонно. И если в do something у нас просто дергается сервис и никакой бизнес логики тут нет, а по сути это роутер или пример реализации одного из слоев миделвари.

У меня есть и роутер самописный, и миддлаври. И всё же, полотно из геттеров - это не нормальный код.

Ну это не ново. Миделвари на базе PSR-15 сильно ограниченные.

Я PSR-15 даже не упоминал.

[peter-gribanov]

И всё же, полотно из геттеров - это не нормальный код.

Вовсе нет. Не все задачи строятся на хендле формочек. Бывает нужно агрегировать некоторые данные из нескольких сервисов и это нормально.

Я PSR-15 даже не упоминал.

О нем говорит Марк в своём докладе на который вы сослались.

[EvgeniiR]

@peter-gribanov

Вовсе нет. Не все задачи строятся на хендле формочек. Бывает нужно агрегировать некоторые данные из нескольких сервисов и это нормально.

Вместо поведения данные - процедурщина во всей красе )

И да - плохи ваши сервисы, если вам данные из них нужно аггрегировать :)

О нем говорит Марк в своём докладе на который вы сослались.

А вы точно его смотрели?)

[peter-gribanov]

И да - плохи ваши сервисы, если вам данные из них нужно аггрегировать :)

А вы слышали о таком понятии как Агрегат?

[EvgeniiR]

@peter-gribanov

А вы слышали о таком понятии как Агрегат?

Слышал, только никакого отношения к аггрегированию данных, агрегат из терминологии DDD - не имеет)

[peter-gribanov]

Слышал, только никакого отношения к аггрегированию данных, агрегат из терминологии DDD - не имеет)

Видимо плохо слышали. Агриграт - это кластер доменных объектов агрегированных в единое целое. Кто агрегирует эти данные и сущности в этот Агригат? Сервис, как правило сервис доменной области. Откуда берутся данные и модели для агрегирования сервиса? Передаются на вход или извлекаются напрямую из других сервисов. Всегда ли данные для агрегата берутся из одного источника? Нет, не всегда. Бывают кейсы в которых агрегат агрегируется из данных полученых из разных источников, служб, сервсиов и микросервисов. То есть, для того, что бы собрать один Агрегат доменной области, иногда нужно сходить в несколько сервсиов и получить (get) из них необходимые данные. И это правильно и нормально если вы придерживаетесь DDD подхода. Поэтому я и говорю, что ваше утверждения некорректно.

И всё же, полотно из геттеров - это не нормальный код.

[EvgeniiR]

@peter-gribanov Не позорься и перечитай что сам кинул, а лучше книжку Еванса. Агрегат просто состоит из нескольких сущностей, никто для него данные не агрегирует. Объект, если хочешь.

Хранение его - это инфраструктура не имеющая значения, загружается агрегат из репозитория.

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

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

А полотно из геттеров - не нормальный код, ни с позиции связности, ни с позиции инкапсуляции, ни с позиции information hiding, ни с позиции GRASP и Infromation Expert, а DDD к этому вообще отношения не имеет, хотя если хотите про геттеры в доменных моделях - вот https://martinfowler.com/bliki/AnemicDomainModel.html .

Далее я продолжать этот диалог не намерен.

[peter-gribanov]

Агрегат просто состоит из нескольких сущностей, никто для него данные не агрегирует.

@EvgeniiR то есть по вашему агрегат из воздуха появляется. Не мелите чушь и лучше сами перечитайте Эванса и Вернона.

хотя если хотите про геттеры в доменных моделях - вот

А у сервисов геттеров быть не может? По вашему если геттер то это анемичная модель и ни, что более? В хоть вдумайтесь, что говорите.

Далее я продолжать этот диалог не намерен.

Поддерживаю. Дискуссия явна зашла в тупик.

[EvgeniiR]

@peter-gribanov

@EvgeniiR то есть по вашему агрегат из воздуха появляется.

Я там явно писал как агрегать может создаваться.

А у сервисов геттеров быть не может? По вашему если геттер то это анемичная модель и ни, что более? В хоть вдумайтесь, что говорите.

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

p.s. https://martinfowler.com/bliki/TellDontAsk.html