-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
4.4. Присвоения в условных операциях #29
Comments
Жуть какая. $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 ...
}
// ... |
Не все if ($token === null) {
// ...
} Ваш вариант выглядит ещё ужасней и приведет к куче дублируемого кода |
почему?
На счет ужасней - очень спорно. На счет дублирования - в каких местах? Если вы про $request и $token - да не вопрос, можно проверки объединить. Если вы про исключения - вот ту не согласен, в каждом случае у вас исключение должно отличаться. |
Потому, что вызов функции обходится дороже чем операция сравнения и статические анализаторы ругаются на такой код.
Если логика программы не считает это исключением, а считает это одним из вариантов нормального состояния программы, то ни какого исключения не будет. В этой программе нам нужно отловить только тот кейс при котором все истино, а все остальные кейсы могут быть проигнорированы. Они нас не интересуют. И в таком случае ваш код превращается в это: $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 или в процессе рефакторинга случайно его удалить и т.д. Много точек выхода из функции. В моем же примере точек выхода всего 2 и как бы код не менялся их число не изменится. |
Можно, но это компенсируется тестами и инспекциями. Правда в вашем примере тоже легко ошибиться, особенно в случае конфликтов при мерджах.
Тут спорный момент, либо у нас большая вложенность и до конца метода тянем результат, либо так. Я не агитирую за то, что прям всегда так надо делать, бывают ситуации, когда минимизация количества выходов вполне оправдана, но на моей практике это встречалось довольно редко.
В вашем пример очень активно используется неявное преобразование типов, которое может к трудно определяемым ошибками. По этой причине он менее безопасен. ($request = $this->request_stack->getMasterRequest()) &&
($session = $request->getSession()) &&
($token = $this->token_storage->getToken()) && В вашем же примере вы не проверяете интерфейсы $request, $session, $token. |
Да, не проверяю потому, что интерфейс мне говорит, что возвращаемое значение |
Тогда наши примеры соответствуют разным условиям)) $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;
} |
@peter-gribanov Я тут провел небольшой бенчмарк по is_null vs === null. Почему-то вызов is_null работает быстрее, хотя по op-кодах вариант с оператором сравнения меньше. https://3v4l.org/WQk7Q |
@peter-gribanov В случае Если я наглупил в бенчмарке - укажите пожалуйста. |
Судя по всему, в последних версиях PHP что-то хорошо оптимизировали. Раньше вызов функции был всегда медленнее: И в вашем примере https://3v4l.org/WQk7Q для PHP версии 7.2.25 и ниже вызов функции медленнее чем операция сравнения. В таком случае, пожалуй стоит использовать функцию |
Что интересно, если добавить |
Добавление глобального неймспейса к is_null убирает поиск по текущему неймспейсу. https://3v4l.org/BcAc0/vld#output Как думаете, есть ли смысл добавить пункт по обязательному указанию глобального неймспейса? |
Я бы не стал. Указывать для каждой функции глобальный неймспейс неудобно. |
@peter-gribanov Тут не присваивание в сравнениях разрешать нужно, а рефакторить код так чтобы этих присваиваний и этих сравненией не было. Все эти проверки в разные миддлвари вынести нужно, а чтобы от присваиваний избавиться, вынести их в отдельный метод, а то и класс. Бенчамарки операторов даже комментировать не хочу - бесполезное это задание, мягко говоря. |
@EvgeniiR миделвари в данном случае это размазывание кода по проекту. |
Давно пора перестать писать весь код в одном файле. |
@EvgeniiR Извините. Случайно отправил недописанное сообщение. Говоря о миделвари вы наверное имели в виду его в контексте CQRS. Миделвари в контексте CQRS хорошая вещь, но она добавляет немного магии и это не всегда хорошо. Это примерно как Event-driven architecture. Хорошо, но в меру. |
Во первых вышеприведённое полотно в любом случае никуда не годится, и речь не про количество строк кода а про количество логики и условий в них.
Никакой CQRS тут не причём, и никакой магии нет. Гляньте на досуге https://www.youtube.com/watch?v=v1I57-_Rsv0 , чтобы не писать 2007-style. И во вторых там нужно всерьёз пересмотреть гранцицы классов, ибо сейчас геттер на геттере сидит и геттером погоняет. |
Как я уже говорил предыдущему комментатору, тут нет ветвления. Тут тот кейс, когда нас интересует только
Ну это не ново. Миделвари на базе PSR-15 сильно ограниченные. |
У меня есть и роутер самописный, и миддлаври. И всё же, полотно из геттеров - это не нормальный код.
Я PSR-15 даже не упоминал. |
Вовсе нет. Не все задачи строятся на хендле формочек. Бывает нужно агрегировать некоторые данные из нескольких сервисов и это нормально.
О нем говорит Марк в своём докладе на который вы сослались. |
Вместо поведения данные - процедурщина во всей красе ) И да - плохи ваши сервисы, если вам данные из них нужно аггрегировать :)
А вы точно его смотрели?) |
А вы слышали о таком понятии как Агрегат? |
Слышал, только никакого отношения к аггрегированию данных, агрегат из терминологии DDD - не имеет) |
Видимо плохо слышали. Агриграт - это кластер доменных объектов агрегированных в единое целое. Кто агрегирует эти данные и сущности в этот Агригат? Сервис, как правило сервис доменной области. Откуда берутся данные и модели для агрегирования сервиса? Передаются на вход или извлекаются напрямую из других сервисов. Всегда ли данные для агрегата берутся из одного источника? Нет, не всегда. Бывают кейсы в которых агрегат агрегируется из данных полученых из разных источников, служб, сервсиов и микросервисов. То есть, для того, что бы собрать один Агрегат доменной области, иногда нужно сходить в несколько сервсиов и получить (get) из них необходимые данные. И это правильно и нормально если вы придерживаетесь DDD подхода. Поэтому я и говорю, что ваше утверждения некорректно.
|
@peter-gribanov Не позорься и перечитай что сам кинул, а лучше книжку Еванса. Хранение его - это инфраструктура не имеющая значения, загружается агрегат из репозитория.
Во первых нет, агрегат либо создаётся новый из пришедших данных извне, либо загружается из репозитория. А полотно из геттеров - не нормальный код, ни с позиции связности, ни с позиции инкапсуляции, ни с позиции information hiding, ни с позиции GRASP и Infromation Expert, а DDD к этому вообще отношения не имеет, хотя если хотите про геттеры в доменных моделях - вот https://martinfowler.com/bliki/AnemicDomainModel.html . Далее я продолжать этот диалог не намерен. |
@EvgeniiR то есть по вашему агрегат из воздуха появляется. Не мелите чушь и лучше сами перечитайте Эванса и Вернона.
А у сервисов геттеров быть не может? По вашему если геттер то это анемичная модель и ни, что более? В хоть вдумайтесь, что говорите.
Поддерживаю. Дискуссия явна зашла в тупик. |
Я там явно писал как агрегать может создаваться.
Я там явно обозначил о чём ссылку кидаю, а геттеры что в сущностях что в сервисах - плохо. Я там даж топиков накидал более конкретных с которыми стоило бы разобраться, а не разводить карго-культ из DDD. |
4.4. Присвоения в условных операциях
Использовать операцию присвоения в условиях конечно не рекомендуется, но бывают кейсы в которых использование присвоения в условиях позволяет избавится от многоуровневой вложенности и сделать код более компактным и читаемым. Хотя это моё субъективное мнение.
VS
Пример посложнее
VS
The text was updated successfully, but these errors were encountered: