Где лучше качество кода?

Question

[Макс]

С коллегой вечный холивар о качестве кода.
Вот один из примеров.
Оцените пожалуйста, как правильней писать в подобных кусочках)

1)

if (!$_SESSION['isMobileOnly']) {
    $opinionList = $dao->getOpinionList($bar['id']);
} else {
    $opinionList = $dao->getOpinionList($bar['id'], 3);
}

2)

$opinionList = $dao->getOpinionList($bar['id'], $_SESSION['isMobileOnly'] ? 3 : null);

3) Свой вариант)

Comments

[Konata Izumi]

Второй лучше

[deepblack]

Количество кураторов тега PHP в ответах зашкаливает )

[Одиночка Айс]

Андрей, да их как грязи сейчас

Answer 1

[Максим Федоров]

Все плохо

• Глобальные переменные — зло
  
• На лицо некий код, который и с сессией работает и с БД — явное нарушение общепринятных принципов, например SRP
  
• $dao как инициализирована? Явно не через конструктор
  
• Использование цифры, вот сиди гадай по ней — это плохо, используйте именованную константу. Например 3 — это некий статус для desktop, вот и назовите ее STATUS_DESKTOP, хотя далее...
  
• вы обращаетесь к одному методу, который шибко умный — работает с флагами, тем самым нарушая SRP и сам содержит if/else, сделайте просто 2 раздельных метода:
  

  getMobileOpinionList()
  getDefaultOpinionList()

  
  
• Предикат лучше вынести в отдельный метод isMobileOnly(), тк условия могут поменяться в нем, не будете же везде в if менять по всему проекту
  

Comments

[Макс]

Это всё понятно, спасибо)
Речь идёт о чистоте, красоте и минималистичности только данного кусочка кода.

[Максим Федоров]

Макс, б этом кусочке и писал :--)

[edward_freedom]

Максим Федоров, зачем ты пишешь двойное тире? Функционал стандартного, одинарного тире не хватило в этом случае?

[Максим Федоров]

edward_freedom, я не поставил до сих пор себе раскладку типографскую,
а привычка — очень-очень сильная вещь

"—" тире, "длинная" черта, часть предложения, отделяется пробелами (т.к. часть предложения)
"-" дефис, короткий символ, часть слова

[DDDsa]

edward_freedom, почему ты пишешь "Функционал"? Это математическое понятие и не имеет никакого отношения к "функциональности" (как следовало написать)

[Александр]

Глобальные переменные — зло

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

[Максим Федоров]

Александр, код, который напрямую работает с глобалками -- не протестировать, тк в тестовой среде (как правило CLI) их в принципе не существует.

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

Есть и еще ряд побочных эффектов

[Александр]

BD_ l3ftoverZ!, Но это же не значит, что мы перестаем работать с глобальной переменной, по крайней мере в самом классе-обёртке. По мне, так это замечание больше похоже на параною, пихать ООП всюду, где нужно и не нужно.
Решение работать с $_SESSION на прямую, или создавать отдельный класс для этого - это вопрос на уровне архитектуры приложения, а не на уровне кода. Если вы работаете с Symfony или аналогом, где есть транспортный уровень и класс для сессии, то да, работать с глобальной переменной будет ошибкой. А вот замечание, что

Глобальные переменные — зло

в данном случае не уместно, как по мне.

[Александр]

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

[edward_freedom]

Максим Федоров, , я проверил минимум 6 страниц твоих ответов, а это почти 1.4 года прошло и у тебя везде стоит два тире

[Максим Федоров]

edward_freedom, ну и что? :) расклалку не ставлю нужную, так решено было давно... Тире имитирую 2 дефисами, привычка...

[edward_freedom]

Максим Федоров, твоя имитация смотрится очень странно, лучше вообще ставь одно тире, суть не меняется совершенно

[Максим Федоров]

edward_freedom, как же, слова слишком близко находятся, в этом задача тире

[edward_freedom]

Максим Федоров, не - близко

[Stalker_RED]

Александр,

больше похоже на параною, пихать ООП всюду, где нужно и не нужно

если вам не нужно, это еще не значит, что оно никому не нужно.
Этот "ООП везде и всюду" нужен для того, чтобы ПОТОМ было легче поддерживать код. Тестировать, менять, дополнять, вот это все.
А не для того, чтобы здесь и сейчас вы написали на 7 буковок меньше.

[Александр]

Stalker_RED,

если вам не нужно, это еще не значит, что оно никому не нужно.

Если оно вам нужно, это еще не значит, что оно нужно каждому. Так спорить можно бесконечно. :)

[Максим Федоров]

Александр, в данном посте речь зашла про качество кода в контексте хороших практик, а не про "кому как удобнее и как любит"

В хороших практиках -- разделение ответственности, возможность тестирования и изоляция от побочных эффектов

[edward_freedom]

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

[Максим Федоров]

edward_freedom,

в чем проблема забиндить этот длинный тире?

Выше ответил, что сделаю, вы правы —это сурагат!

ставишь даже там, где оно в принципе не надо, даже одно.

Можно пример, где не правильно применил — исправлю :)

ты двойное тире

Двойной дефис я использую как тире, а дефис как дефис, это две разные вещи:

• дефис: где-то, как-то, еле-еле
  
• тире: Москва — столица России, в моем случае я писал тут "--"
  

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

Прошу прощения, если чем-то насолил. Цель куратора — решать вопросы по тегу, если для вас это огромная проблема, я все поправлю в ближайшее время. Еще раз приношу извинения.
Наоборот — стараюсь верно следовать этому мало-мальски важному принципу русского языка

[edward_freedom]

Максим Федоров, разница между - и — пару пикселей, никто не заметит разницу, а между — и -- разница есть и бросается в глаза в ту же секунду.
А по поводу куратора тега. Ты как представительское лицо, приближенное к административному составу и представляешь форум, должен же как то писать по ГОСТу языка, а не выдумывать что то свое.

[Максим Федоров]

edward_freedom, ваша претензия понятна :)
По ГОСТ-у языка — использовать тире, вместо дефиса там, где нужно. У меня оно просто кривое :(
Еще раз извиняюсь — исправлю на тире со временем во всех ответах. Куратор я недавно... В доброй сотне последних ответов точно поправлю.

[profesor08]

edward_freedom,

представительское лицо, приближенное к административному составу

это не так

[edward_freedom]

profesor08, раз он получил статус, отличный от обычного пользователя, то он уже начал являться частью форума, представляющий непосредственно этот самый форум

[profesor08]

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

Автор вопроса тоже может отметить решение решением, и у него тоже отличный статус от остальных.

[edward_freedom]

profesor08, ты путаешь. Все аккаунты вконтакте, это пользователи. Если пользователь на стене будет писать гадости, никто не обратит на это внимание. А если будут писать работники вконтакте, то это уже будет иметь последствия

[profesor08]

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

[DevMan]

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

[Михаил Братеньков]

Уточните как в данном месте нарушается SRP и как его исправить

Допустим у нас есть код

class MyController extends Controller 
{
    public function actionIndex()
    {
        if (!$_SESSION['isMobileOnly']) {
            $opinionList = $dao->getOpinionList($bar['id']);
        } else {
            $opinionList = $dao->getOpinionList($bar['id'], 3);
        }
        return $this->render('MyView', $opinionList)
    }
}

Что в данном случае можно вынести в отдельный класс/функцию?

Получение данных из БД? Они и так вынесены в ORM.

Работа с сессией? Каким образом тогда проверять значение isMobileOnly?

UPD: Конечно можно вынести работу с сессией в какой-нибудь специальный класс и обращаться к нему например Session->get('isMobileOnly') как вы предлагали выше, это облегчит редактирование кода и позволит написать юнит-тесты, но с точки зрения SRP ведь ничего не изменится

[Максим Федоров]

Михаил Братеньков,
возможно, что я лишканул про пункт "с сессией и с БД работает и нарушает SRP"

1. SRP нарушает как минимум метод getOpinionList()

public function getOpinionList($bar, int $anyFlag)
{
     if ($anyFlag === 3) {
          // делаем одно для всех устройств
     } else {
          // делаем другое как минимум для мобилок, а может и еще хулиганим из-за этого флага
     } 
}

2. У нас есть некий предикат:
if (!$_SESSION['isMobileOnly'])

Сейчас у нас есть некое условие, которое созвучно с тем, что написано в нем, но может ведь усложнить/поменяться, более того может переиспользоваться в самом классе (в др экшнах к примеру), хорошей практикой является вынос предикатов в отдельный метод (я бы вообще вынес в другой объект)
if ($this->visitor->isMobileOnly())

3. И все же сожалению видно, что контроллер шибко умный и шибко большой — не просто запрос обрабатывает, но еще и чем-то другим занимается ($dao и $bar не указанно как появляется, особенно видно, что $bar уже массив, то есть есть мнение, что не из Request прилетает)

[edward_freedom]

DevMan, даже если опустить, что он не обычный пользователь, а считать его за обычного, то правила все равно для всех сделаны:

3.6. Соблюдать нормы русского языка и принципы построения вопросительных предложений.

[Максим Федоров]

edward_freedom, ты очень вгрызся в написание моего тире, я не писал дефис (как это делают все), что более неверно, при том я не настаиваю, что так должны делать другие. Просто имитировал одно тире написанием двумя дефисами — старался сделать именно по правилам, но вам написание не понравилось. Принял доводы ваши, но вы прямо чморить стали, что, видимо, и расстраивает.

[edward_freedom]

Максим Федоров, я уже давно закрыл вопрос, это он продолжает обсуждать

[Максим Федоров]

edward_freedom,

Кроме того я много переписываюсь в Телеграме и ВК:

• Телега прямо во время ввода меняет 2 дефиса на тире
  
• ВК после отправки
  

Answer 2

[Immortal_pony]

$deviceCode = $_SESSION['isMobileOnly'] ? 3 : null;
$opinionList = $dao->getOpinionList($bar['id'], $deviceCode);

Comments

[Макс]

Почему лучше в 2 строчки?
Далее переменная $deviceCode нигде не будет использоваться.

[Immortal_pony]

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

[Северное Сияние]

Макс,

Далее переменная $deviceCode нигде не будет использоваться.

ну и что? это ни на что не влияет. зато повышается читабельность кода.

[Михаил Братеньков]

Макс,

Далее переменная $deviceCode нигде не будет использоваться.

Она будет использоваться следующим программистом когда он будет рефакторить код и пытаться понять что такое 3 и почему именно 3

Answer 3

[dkar007]

Первый вариант визуально проще разобрать, чем второй.

Answer 4

[Александр Владимирович]

Oбернуть сессию в объект, использовать типы везде, где это возможно.

См. https://symfony.com/doc/current/components/http_fo...

p.s. число 3 вынести в именованную константу.

Answer 5

[Adamos]

В порядке увеличения энтропии:

$opinionList = $dao->getOpinionList(
    $bar['id'], 
    $_SESSION['isMobileOnly'] ? 3 : null
);

Answer 6

[Арслан Исаев]

Ни один! Что за число 3, почему не вынесена в константу, если она так важна. Как будто вы пишите только для себя..