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]

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

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